[Rt-commit] rt branch, 4.2/csrf-whitelist, updated. rt-4.2.11-39-g2f6d9a3

[Dustin Graves]

The branch, 4.2/csrf-whitelist has been updated
       via  2f6d9a303f3046c96d662fb916e6667c999df146 (commit)
      from  28d388288fa67217aa801e27cd63044182470f86 (commit)

Summary of changes:
 lib/RT/Interface/Web.pm | 58 +++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 24 deletions(-)

- Log -----------------------------------------------------------------
commit 2f6d9a303f3046c96d662fb916e6667c999df146
Author: Dustin Graves <dustin at bestpractical.com>
Date:   Mon Aug 3 17:09:47 2015 -0400

    move CSRF parameter whitelist logic into own sub

diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index b633aaf..8e93e43 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1376,8 +1376,27 @@ our %is_whitelisted_component = (
     '/Ticket/ShowEmailRecord.html' => 1,
 );
 
-our %is_whitelisted_component_parameter = (
-    '/Search/Build.html' => { 'SavedSearchLoad' => 1 },
+# Whitelist arguments that do not indicate an effectful request.
+our @whitelisted_args = (
+    # For example, "id" is acceptable because that is how RT retrieves a
+    # record.
+    'id' => 1,
+
+    # If they have a results= from MaybeRedirectForResults, that's also fine.
+    'results' => 1,
+
+    # The homepage refresh, which uses the Refresh header, doesn't send
+    # a referer in most browsers; whitelist the one parameter it reloads
+    # with, HomeRefreshInterval, which is safe
+    'HomeRefreshInterval' => 1,
+
+    # The NotMobile flag is fine for any page; it's only used to toggle a flag
+    # in the session related to which interface you get.
+    'NotMobile' => 1,
+);
+
+our %whitelisted_component_parameters = (
+    '/Search/Build.html' => ['SavedSearchLoad'],
 );
 
 # Components which are blacklisted from automatic, argument-based whitelisting.
@@ -1424,34 +1443,25 @@ sub IsCompCSRFWhitelisted {
         }
     }
 
-    # Eliminate arguments that do not indicate an effectful request.
-    # For example, "id" is acceptable because that is how RT retrieves a
-    # record.
-    delete $args{id};
+    return AreCompCSRFParametersWhitelisted($comp, $ARGS);
+}
 
-    # If they have a results= from MaybeRedirectForResults, that's also fine.
-    delete $args{results};
+sub AreCompCSRFParametersWhitelisted {
+    my $sub = shift;
+    my $ARGS = shift;
 
-    # The homepage refresh, which uses the Refresh header, doesn't send
-    # a referer in most browsers; whitelist the one parameter it reloads
-    # with, HomeRefreshInterval, which is safe
-    delete $args{HomeRefreshInterval};
+    my %args = %{ $ARGS };
 
-    # The NotMobile flag is fine for any page; it's only used to toggle a flag
-    # in the session related to which interface you get.
-    delete $args{NotMobile};
+    my %blacklisted_args = %args;
 
-    # Whitelist compontent parameters
-    my %is_whitelisted_arg = %{$is_whitelisted_component_parameter{$comp}};
-    for my $arg (keys %is_whitelisted_arg) {
-        delete $args{$arg} if $is_whitelisted_arg{$arg};
-    }
+    # Join global whitelist and component-specific whitelist
+    my @comp_whitelisted_args = (@whitelisted_args, @{$whitelisted_component_parameters{$sub}});
 
-    # If there are no arguments, then it's likely to be an idempotent
-    # request, which are not susceptible to CSRF
-    return 1 if !%args;
+    for my $arg (@comp_whitelisted_args) {
+        delete $blacklisted_args{$arg};
+    }
 
-    return 0;
+    return !%blacklisted_args;
 }
 
 sub IsRefererCSRFWhitelisted {

-----------------------------------------------------------------------