Bug 1426409 - github_secret key has no rate limiting

4 files changed, 42 insertions, 16 deletions

diff --git a/Bugzilla.pm b/Bugzilla.pm
index 9e5177839..b7aaadf3f 100644
--- a/Bugzilla.pm
+++ b/Bugzilla.pm
@@ -317,7 +317,7 @@ sub github_secret {
     my $cache = $class->request_cache;
     my $cgi   = $class->cgi;
 
-    $cache->{github_secret} //= $cgi->cookie('github_secret') // generate_random_password(16);
+    $cache->{github_secret} //= $cgi->cookie('github_secret') // generate_random_password(256);
 
     return $cache->{github_secret};
 }
@@ -853,7 +853,7 @@ sub check_rate_limit {
                 $action = 'ignore';
             }
             my $limit = join("/", @$limit);
-            Bugzilla->audit("[rate_limit] action=$action, ip=$ip, limit=$limit");
+            Bugzilla->audit("[rate_limit] action=$action, ip=$ip, limit=$limit, name=$name");
             ThrowUserError("rate_limit") if $action eq 'block';
         }
     }
diff --git a/Bugzilla/Config.pm b/Bugzilla/Config.pm
index f93992f91..d050ff9e0 100644
--- a/Bugzilla/Config.pm
+++ b/Bugzilla/Config.pm
@@ -193,6 +193,20 @@ sub update_params {
                 $param->{$name} = $item->{'default'};
             }
         }
+        else {
+            my $checker = $item->{'checker'};
+            my $updater = $item->{'updater'};
+            if ($checker) {
+                my $error = $checker->($param->{$name}, $item);
+                if ($error && $updater) {
+                    my $new_val = $updater->( $param->{$name} );
+                    $param->{$name} = $new_val unless $checker->($new_val, $item);
+                }
+                elsif ($error) {
+                    warn "Invalid parameter: $name\n";
+                }
+            }
+        }
     }
 
     # Generate unique Duo integration secret key
diff --git a/Bugzilla/Config/Admin.pm b/Bugzilla/Config/Admin.pm
index ad24f7112..ac1c4ca0e 100644
--- a/Bugzilla/Config/Admin.pm
+++ b/Bugzilla/Config/Admin.pm
@@ -12,7 +12,7 @@ use strict;
 use warnings;
 
 use Bugzilla::Config::Common;
-use JSON::XS qw(decode_json);
+use JSON::XS qw(decode_json encode_json);
 use List::MoreUtils qw(all);
 use Scalar::Util qw(looks_like_number);
 
@@ -55,8 +55,9 @@ sub get_param_list {
         {
             name    => 'rate_limit_rules',
             type    => 'l',
-            default => '{"get_bug": [75, 60], "show_bug": [75, 60]}',
+            default => '{"get_bug": [75, 60], "show_bug": [75, 60], "github": [10, 60]}',
             checker => \&check_rate_limit_rules,
+            updater => \&update_rate_limit_rules,
         },
 
         {
@@ -78,11 +79,18 @@ sub check_rate_limit_rules {
         ref($_) eq 'ARRAY' && looks_like_number( $_->[0] ) && looks_like_number( $_->[1] )
     } values %$val;
 
-    foreach my $required (qw( show_bug get_bug )) {
+    foreach my $required (qw( show_bug get_bug github )) {
         return "missing $required" unless exists $val->{$required};
     }
 
     return "";
 }
 
+sub update_rate_limit_rules {
+    my ($rules) = @_;
+    my $val = decode_json($rules);
+    $val->{github} = [10, 60];
+    return encode_json($val);
+}
+
 1;
diff --git a/github.cgi b/github.cgi
index b8467e1e0..f280f6ac9 100755
--- a/github.cgi
+++ b/github.cgi
@@ -13,7 +13,7 @@ use warnings;
 use lib qw(. lib local/lib/perl5);
 
 use Bugzilla;
-use Bugzilla::Util ();
+use Bugzilla::Util qw(remote_ip);
 use Bugzilla::Error;
 use Bugzilla::Constants;
 use Bugzilla::Token qw( issue_short_lived_session_token
@@ -37,8 +37,10 @@ if (lc($cgi->request_method) eq 'post') {
     my $github_secret  = $cgi->param('github_secret') or ThrowCodeError("github_invalid_request", { reason => 'invalid secret' });
     my $github_secret2 = Bugzilla->github_secret      or ThrowCodeError("github_invalid_request", { reason => 'invalid secret' });
 
-    ThrowCodeError("github_invalid_request", { reason => 'invalid secret' })
-      unless $github_secret eq $github_secret2;
+    if ($github_secret ne $github_secret2) {
+        Bugzilla->check_rate_limit('github', remote_ip());
+        ThrowCodeError("github_invalid_request", { reason => 'invalid secret' });
+    }
 
     ThrowCodeError("github_invalid_target", { target_uri => $target_uri })
       unless $target_uri =~ /^\Q$urlbase\E/;
@@ -71,13 +73,18 @@ elsif (lc($cgi->request_method) eq 'get') {
         exit;
     }
 
-    ThrowCodeError("github_invalid_request", { reason => 'invalid state param' })
-      unless $state_param eq $state_cookie;
+    my $invalid_request = $state_param ne $state_cookie;
 
-    my $state_data = get_token_extra_data($state_param);
-    ThrowCodeError("github_invalid_request", { reason => 'invalid state param' } )
-      unless $state_data && $state_data->{type};
+    my $state_data;
+    unless ($invalid_request) {
+        $state_data = get_token_extra_data($state_param);
+        $invalid_request = !( $state_data && $state_data->{type} && $state_data->{type} =~ /^github_(?:login|email)$/  );
+    }
 
+    if ($invalid_request) {
+        Bugzilla->check_rate_limit('github', remote_ip());
+        ThrowCodeError("github_invalid_request", { reason => 'invalid state param' } )
+    }
 
     $cgi->remove_cookie('github_state');
     delete_token($state_param);
@@ -90,9 +97,6 @@ elsif (lc($cgi->request_method) eq 'get') {
         Bugzilla->request_cache->{github_action} = 'email';
         Bugzilla->request_cache->{github_emails} = $state_data->{emails};
     }
-    else {
-        ThrowCodeError("github_invalid_request", { reason => "invalid state param" })
-    }
     my $user = Bugzilla->login(LOGIN_REQUIRED);
 
     my $target_uri = URI->new($state_data->{target_uri});