Attachment 167560 Details for Bug 37040 – Bug 37040: Prevent ErrorDocument subrequests from activating CSRF

[patch] Bug 37040: Prevent ErrorDocument subrequests from activating CSRF

Bug-37040-Prevent-ErrorDocument-subrequests-from-a.patch (text/plain), 2.41 KB, created by David Nind on 2024-06-06 22:12:31 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Nind

Created: 2024-06-06 22:12:31 UTC

Size: 2.41 KB

patch

obsolete

>From ec6edce35310274b35e2ccb40f6e77ae7947932b Mon Sep 17 00:00:00 2001
>From: David Cook <dcook@prosentient.com.au>
>Date: Thu, 6 Jun 2024 01:34:23 +0000
>Subject: [PATCH] Bug 37040: Prevent ErrorDocument subrequests from activating
> CSRF
>
>This change improves the mechanism for preventing the CSRF middleware
>being activated by ErrorDocument subrequests.
>
>This change was necessary due to a subtle issue identified by
>Bug 37041.
>
>Test plan:
>0. Apply the patch
>1. Restart Koha
>koha-plack --restart kohadev
>2. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=9908
>3. Log in
>4. Note that you get a pretty 403 and not an ugly plain text error
>5. Go to http://localhost:8081
>6. Fill in the login details, but use the HTML inspector to delete
>the csrf_token from the hidden inputs
>7. Submit the login
>8. Note a pretty 403 page
>
>Signed-off-by: David Nind <david@davidnind.com>
>---
> Koha/Middleware/CSRF.pm | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/Koha/Middleware/CSRF.pm b/Koha/Middleware/CSRF.pm
>index b34208ec6c..8850d0ef80 100644
>--- a/Koha/Middleware/CSRF.pm
>+++ b/Koha/Middleware/CSRF.pm
>@@ -43,6 +43,11 @@ sub call {
>     my $uri            = $req->uri     // q{};
>     my $referer        = $req->referer // q{No referer};
> 
>+    #NOTE: Ignore ErrorDocument requests for CSRF
>+    if ( $env->{'psgix.errordocument.SCRIPT_NAME'} ) {
>+        return $self->app->($env);
>+    }
>+
>     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 %s (referer: %s)", $original_op,
>@@ -73,14 +78,9 @@ sub call {
>         }
>     }
> 
>-    #NOTE: It is essential to check for this environmental variable.
>-    #NOTE: If we don't check for it, then we'll also throw an error for the "subrequest" that ErrorDocument uses to
>-    #fetch the error page document. Then we'll wind up with a very ugly error page and not our pretty one.
>-    if ( $error && !$env->{'plack.middleware.Koha.CSRF'} ) {
>-
>+    if ( $error ) {
>         #NOTE: Other Middleware will take care of logging to correct place, as Koha::Logger doesn't know where to go here
>         warn $error;
>-        $env->{'plack.middleware.Koha.CSRF'} = $error;
>         my $res = Plack::Response->new( 403, [ 'Content-Type' => 'text/plain' ], ["Wrong CSRF token"] );
>         return $res->finalize;
>     }
>-- 
>2.39.2

Actions: View | Diff | Splinter Review

Attachments on bug 37040: 167521 | 167560 | 167571 | 167572 | 167620