Bug 36148 - Move CSRF check code outside of CGI->new
Summary: Move CSRF check code outside of CGI->new
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 34478
Blocks: 36192 36177 36358
  Show dependency treegraph
 
Reported: 2024-02-22 09:48 UTC by Jonathan Druart
Modified: 2024-04-17 08:49 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 36148: Move CSRF check to a Plack middleware (3.63 KB, patch)
2024-02-22 14:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36148: Move CSRF check to a Plack middleware (8.45 KB, patch)
2024-02-22 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36148: Allow Koha::Middleware::CSRF to use error/exception middlewares (5.81 KB, patch)
2024-02-23 05:07 UTC, David Cook
Details | Diff | Splinter Review
Bug 36148: Add explanatory notes (975 bytes, patch)
2024-02-23 05:15 UTC, David Cook
Details | Diff | Splinter Review
Bug 36148: Improve error handling and restore programming errors (7.81 KB, patch)
2024-02-23 07:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36148: Fix header name (972 bytes, patch)
2024-02-23 08:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36148: Do not deal with CSRF error in get_template_and_user (1.10 KB, patch)
2024-02-26 10:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36148: Move CSRF check to a Plack middleware (8.51 KB, patch)
2024-02-27 15:19 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36148: Allow Koha::Middleware::CSRF to use error/exception middlewares (5.88 KB, patch)
2024-02-27 15:19 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36148: Add explanatory notes (1.01 KB, patch)
2024-02-27 15:19 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36148: Improve error handling and restore programming errors (7.87 KB, patch)
2024-02-27 15:19 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36148: Fix header name (1.00 KB, patch)
2024-02-27 15:19 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36148: Do not deal with CSRF error in get_template_and_user (1.16 KB, patch)
2024-02-27 15:19 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2024-02-22 09:48:02 UTC
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.
Comment 1 Marcel de Rooy 2024-02-22 10:38:57 UTC
(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" );
        }
Comment 2 Marcel de Rooy 2024-02-22 10:46:42 UTC
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?
Comment 4 Jonathan Druart 2024-02-22 13:32:49 UTC
(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.
Comment 5 Jonathan Druart 2024-02-22 14:20:45 UTC
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.
Comment 6 Jonathan Druart 2024-02-22 14:22:49 UTC
To apply on top of security/bug_34478_svc (if not there yet).
Comment 7 Jonathan Druart 2024-02-22 14:50:15 UTC
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.
Comment 9 David Cook 2024-02-23 04:33:33 UTC
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...
Comment 10 David Cook 2024-02-23 05:07:31 UTC
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.
Comment 11 David Cook 2024-02-23 05:10:04 UTC
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...
Comment 12 David Cook 2024-02-23 05:14:37 UTC
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...
Comment 13 David Cook 2024-02-23 05:15:46 UTC
Created attachment 162360 [details] [review]
Bug 36148: Add explanatory notes
Comment 14 David Cook 2024-02-23 05:24:13 UTC
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.
Comment 15 Jonathan Druart 2024-02-23 07:47:34 UTC
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.
Comment 16 Jonathan Druart 2024-02-23 07:54:37 UTC
Created attachment 162362 [details] [review]
Bug 36148: Improve error handling and restore programming errors
Comment 17 Jonathan Druart 2024-02-23 08:07:37 UTC
Created attachment 162363 [details] [review]
Bug 36148: Fix header name
Comment 18 Jonathan Druart 2024-02-23 08:25:10 UTC
We may want to return a JSON response instead of HTML is the client is asking for it.
Comment 19 Nick Clemens (kidclamp) 2024-02-23 15:36:43 UTC
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
Comment 20 David Cook 2024-02-26 03:46:47 UTC
(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.
Comment 21 Jonathan Druart 2024-02-26 10:32:27 UTC
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.
Comment 22 Jonathan Druart 2024-02-26 10:33:15 UTC
(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.
Comment 23 Jonathan Druart 2024-02-26 10:33:53 UTC
403 is now looking like that: https://snipboard.io/Pr0cFd.jpg
Comment 24 David Cook 2024-02-26 22:13:49 UTC
(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. 👍
Comment 25 Nick Clemens (kidclamp) 2024-02-27 15:19:26 UTC
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>
Comment 26 Nick Clemens (kidclamp) 2024-02-27 15:19:28 UTC
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>
Comment 27 Nick Clemens (kidclamp) 2024-02-27 15:19:31 UTC
Created attachment 162507 [details] [review]
Bug 36148: Add explanatory notes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Nick Clemens (kidclamp) 2024-02-27 15:19:33 UTC
Created attachment 162508 [details] [review]
Bug 36148: Improve error handling and restore programming errors

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Nick Clemens (kidclamp) 2024-02-27 15:19:36 UTC
Created attachment 162509 [details] [review]
Bug 36148: Fix header name

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Nick Clemens (kidclamp) 2024-02-27 15:19:38 UTC
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>
Comment 31 Jonathan Druart 2024-03-01 13:23:54 UTC
Pushed to master for 24.05.00.
Comment 32 Fridolin Somers 2024-04-17 08:49:54 UTC
Depends on Bug 34478 not in 23.11.x