From 4bd4b367dd1b23c8917aa0f1b5c655d49aaae063 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 22 Feb 2024 15:16:08 +0100 Subject: [PATCH] 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: Jonathan Druart --- C4/Auth.pm | 13 +-- Koha/Middleware/CSRF.pm | 82 +++++++++++++++++++ debian/templates/plack.psgi | 51 +----------- .../prog/en/includes/messages.inc | 3 - 4 files changed, 86 insertions(+), 63 deletions(-) create mode 100644 Koha/Middleware/CSRF.pm diff --git a/C4/Auth.pm b/C4/Auth.pm index d1a8235b56..a407ba1a3c 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -484,14 +484,6 @@ sub get_template_and_user { OPACBaseURL => C4::Context->preference('OPACBaseURL'), minPasswordLength => $minPasswordLength, ); - if ( C4::Context->config('dev_install') ) { - # Dealing with debug_programming_error ( set from plack.psgi) - $template->param( - dev_install => 1, - debug_programming_error => scalar $in->{query}->param('debug_programming_error'), - ); - $in->{query}->delete('debug_programming_error'); - } if ( $in->{'type'} eq "intranet" ) { $template->param( @@ -648,9 +640,8 @@ sub get_template_and_user { $template->param( logged_in_user => $patron ); $template->param( sessionID => $sessionID ); - if ( $in->{query}->param('invalid_csrf_token') ) { - Koha::Logger->get->debug("The form submission failed (Wrong CSRF token)."); - C4::Output::output_and_exit( $in->{query}, $cookie, $template, 'wrong_csrf_token' ); + if ( $ENV{KOHA_ERROR} ) { + C4::Output::output_and_exit( $in->{query}, $cookie, $template, $ENV{KOHA_ERROR} ); } return ( $template, $borrowernumber, $cookie, $flags ); diff --git a/Koha/Middleware/CSRF.pm b/Koha/Middleware/CSRF.pm new file mode 100644 index 0000000000..7b9d43590f --- /dev/null +++ b/Koha/Middleware/CSRF.pm @@ -0,0 +1,82 @@ +package Koha::Middleware::CSRF; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use parent qw(Plack::Middleware); + +use Koha::Logger; + +sub call { + my ( $self, $env ) = @_; + my $req = Plack::Request->new($env); + + my %stateless_methods = ( + GET => 1, + HEAD => 1, + OPTIONS => 1, + TRACE => 1, + ); + + my %stateful_methods = ( + POST => 1, + PUT => 1, + DELETE => 1, + PATCH => 1, + ); + + my $original_op = $req->param('op'); + my $request_method = $req->method // q{}; + my ( $error ); + if ( $stateless_methods{$request_method} && defined $original_op && $original_op =~ m{^cud-} ) { + $error = sprintf "Programming error - op '%s' must not start with 'cud-' for %s", $original_op, + $request_method; + } elsif ( $stateful_methods{$request_method} ) { + + # Get the CSRF token from the param list or the header + my $csrf_token = $req->param('csrf_token') || $req->header('HTTP_CSRF_TOKEN'); + + if ( defined $req->param('op') && $original_op !~ m{^cud-} ) { + $error = sprintf "Programming error - op '%s' must start with 'cud-' for %s", $original_op, + $request_method; + } elsif ( !$csrf_token ) { + $error = sprintf "Programming error - No CSRF token passed for %s", $request_method; + } else { + unless ( + Koha::Token->new->check_csrf( + { + session_id => scalar $req->cookies->{CGISESSID}, + token => $csrf_token, + } + ) + ) + { + $error = "wrong_csrf_token"; + } + } + } + + if ( $error ) { + Koha::Logger->get->warn( $error ); + $env->{KOHA_ERROR} = $error; + $env->{PATH_INFO} = '/intranet/errors/403.pl'; + } + + return $self->app->($env); +} + +1; diff --git a/debian/templates/plack.psgi b/debian/templates/plack.psgi index dee2e2f4ed..fd968da1a4 100644 --- a/debian/templates/plack.psgi +++ b/debian/templates/plack.psgi @@ -47,55 +47,6 @@ use CGI qw(-utf8 ); # we will loose -utf8 under plack, otherwise $CGI::PARAM_UTF8 = 1; Koha::Caches->flush_L1_caches(); Koha::Cache::Memory::Lite->flush(); - - my %stateless_methods = ( - GET => 1, - HEAD => 1, - OPTIONS => 1, - TRACE => 1, - ); - - my %stateful_methods = ( - POST => 1, - PUT => 1, - DELETE => 1, - PATCH => 1, - ); - - 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-} ) { - Koha::Logger->get->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} ) { - # Get the CSRF token from the param list or the header - my $csrf_token = $q->param('csrf_token') || $q->http('HTTP_CSRF_TOKEN'); - - if ( defined $q->param('op') && $original_op !~ m{^cud-} ) { - Koha::Logger->get->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" ); - } - - die "Programming error - No CSRF token passed for $request_method" - unless $csrf_token; - - unless ( - Koha::Token->new->check_csrf( - { - session_id => scalar $q->cookie('CGISESSID'), - token => $csrf_token, - } - ) - ) - { - Koha::Logger->get->debug("The form submission failed (Wrong CSRF token)."); - $q->param( 'op', '' ); - $q->param( 'invalid_csrf_token', 1); - } - } - return $q; }; } @@ -126,6 +77,7 @@ builder { enable "+Koha::Middleware::RealIP"; mount '/opac' => builder { + enable "+Koha::Middleware::CSRF"; #NOTE: it is important that these are relative links enable 'ErrorDocument', 400 => 'errors/400.pl', @@ -145,6 +97,7 @@ builder { $opac; }; mount '/intranet' => builder { + enable "+Koha::Middleware::CSRF"; #NOTE: it is important that these are relative links enable 'ErrorDocument', 400 => 'errors/400.pl', diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/messages.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/messages.inc index 5802138616..34ebe5c6e7 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/messages.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/messages.inc @@ -1,6 +1,3 @@
[% INCLUDE 'blocking_errors.inc' %] - [% IF dev_install && debug_programming_error %] -
Programming error: [% debug_programming_error %]
- [% END %]
-- 2.39.5