From 11e9557ccb3df968ede8f863880a71e5155330b6 Mon Sep 17 00:00:00 2001 From: Lucas Mansur Date: Fri, 30 Mar 2018 16:08:00 -0300 Subject: [PATCH] [Fixes #302] Initial style guide adoption (#330) * Initial Rubocop configuration * Fix Rubocop layout offenses for lib * Fix some spec offenses * Fix leftover layout offenses --- .rubocop.yml | 5 +++++ Guardfile | 1 - lib/rack/attack.rb | 17 ++++++++--------- lib/rack/attack/allow2ban.rb | 1 + lib/rack/attack/blocklist.rb | 1 - lib/rack/attack/cache.rb | 1 - lib/rack/attack/check.rb | 3 +-- lib/rack/attack/fail2ban.rb | 3 ++- lib/rack/attack/path_normalizer.rb | 12 +++++------- lib/rack/attack/safelist.rb | 1 - lib/rack/attack/store_proxy.rb | 2 +- lib/rack/attack/store_proxy/dalli_proxy.rb | 5 ++--- lib/rack/attack/store_proxy/mem_cache_proxy.rb | 9 ++++----- .../attack/store_proxy/redis_store_proxy.rb | 6 +++--- rack-attack.gemspec | 1 + spec/acceptance/safelisting_ip_spec.rb | 1 - spec/allow2ban_spec.rb | 12 ++++++------ spec/fail2ban_spec.rb | 14 +++++++------- spec/rack_attack_dalli_proxy_spec.rb | 2 -- spec/rack_attack_spec.rb | 12 ++++++------ spec/rack_attack_throttle_spec.rb | 10 +++++----- spec/rack_attack_track_spec.rb | 8 ++++---- spec/spec_helper.rb | 5 ++--- 23 files changed, 63 insertions(+), 69 deletions(-) create mode 100644 .rubocop.yml diff --git a/.rubocop.yml b/.rubocop.yml new file mode 100644 index 0000000..cf91fe2 --- /dev/null +++ b/.rubocop.yml @@ -0,0 +1,5 @@ +AllCops: + DisabledByDefault: true + +Layout: + Enabled: true diff --git a/Guardfile b/Guardfile index ebf900b..06b986b 100644 --- a/Guardfile +++ b/Guardfile @@ -7,4 +7,3 @@ guard :minitest do watch(%r{^lib/(.+)\.rb$}) { |m| "spec/#{m[1]}_spec.rb" } watch(%r{^spec/spec_helper\.rb$}) { 'spec' } end - diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb index 35b2e35..ad5e1eb 100644 --- a/lib/rack/attack.rb +++ b/lib/rack/attack.rb @@ -22,7 +22,6 @@ class Rack::Attack autoload :Allow2Ban, 'rack/attack/allow2ban' class << self - attr_accessor :notifier, :blocklisted_response, :throttled_response def safelist(name, &block) @@ -64,8 +63,11 @@ class Rack::Attack end def safelists; @safelists ||= {}; end + def blocklists; @blocklists ||= {}; end + def throttles; @throttles ||= {}; end + def tracks; @tracks ||= {}; end def whitelists @@ -126,7 +128,7 @@ class Rack::Attack def blacklisted_response=(res) warn "[DEPRECATION] 'Rack::Attack.blacklisted_response=' is deprecated. Please use 'blocklisted_response=' instead." - self.blocklisted_response=(res) + self.blocklisted_response = res end def blacklisted_response @@ -147,10 +149,10 @@ class Rack::Attack # Set defaults @notifier = ActiveSupport::Notifications if defined?(ActiveSupport::Notifications) - @blocklisted_response = lambda {|env| [403, {'Content-Type' => 'text/plain'}, ["Forbidden\n"]] } - @throttled_response = lambda {|env| + @blocklisted_response = lambda { |env| [403, { 'Content-Type' => 'text/plain' }, ["Forbidden\n"]] } + @throttled_response = lambda { |env| retry_after = (env['rack.attack.match_data'] || {})[:period] - [429, {'Content-Type' => 'text/plain', 'Retry-After' => retry_after.to_s}, ["Retry later\n"]] + [429, { 'Content-Type' => 'text/plain', 'Retry-After' => retry_after.to_s }, ["Retry later\n"]] } def initialize(app) @@ -174,8 +176,5 @@ class Rack::Attack end extend Forwardable - def_delegators self, :safelisted?, - :blocklisted?, - :throttled?, - :tracked? + def_delegators self, :safelisted?, :blocklisted?, :throttled?, :tracked? end diff --git a/lib/rack/attack/allow2ban.rb b/lib/rack/attack/allow2ban.rb index 763253b..9a91086 100644 --- a/lib/rack/attack/allow2ban.rb +++ b/lib/rack/attack/allow2ban.rb @@ -3,6 +3,7 @@ module Rack class Allow2Ban < Fail2Ban class << self protected + def key_prefix 'allow2ban' end diff --git a/lib/rack/attack/blocklist.rb b/lib/rack/attack/blocklist.rb index b61a29f..3cfba54 100644 --- a/lib/rack/attack/blocklist.rb +++ b/lib/rack/attack/blocklist.rb @@ -5,7 +5,6 @@ module Rack super @type = :blocklist end - end end end diff --git a/lib/rack/attack/cache.rb b/lib/rack/attack/cache.rb index 231b5c5..0e6e6d3 100644 --- a/lib/rack/attack/cache.rb +++ b/lib/rack/attack/cache.rb @@ -1,7 +1,6 @@ module Rack class Attack class Cache - attr_accessor :prefix def initialize diff --git a/lib/rack/attack/check.rb b/lib/rack/attack/check.rb index 8ae9ba1..49857cf 100644 --- a/lib/rack/attack/check.rb +++ b/lib/rack/attack/check.rb @@ -8,7 +8,7 @@ module Rack end def [](req) - block[req].tap {|match| + block[req].tap { |match| if match req.env["rack.attack.matched"] = name req.env["rack.attack.match_type"] = type @@ -21,4 +21,3 @@ module Rack end end end - diff --git a/lib/rack/attack/fail2ban.rb b/lib/rack/attack/fail2ban.rb index 76dc59f..413808e 100644 --- a/lib/rack/attack/fail2ban.rb +++ b/lib/rack/attack/fail2ban.rb @@ -27,6 +27,7 @@ module Rack end protected + def key_prefix 'fail2ban' end @@ -40,8 +41,8 @@ module Rack true end - private + def ban!(discriminator, bantime) cache.write("#{key_prefix}:ban:#{discriminator}", 1, bantime) end diff --git a/lib/rack/attack/path_normalizer.rb b/lib/rack/attack/path_normalizer.rb index f4e686f..fd5935f 100644 --- a/lib/rack/attack/path_normalizer.rb +++ b/lib/rack/attack/path_normalizer.rb @@ -1,5 +1,4 @@ class Rack::Attack - # When using Rack::Attack with a Rails app, developers expect the request path # to be normalized. In particular, trailing slashes are stripped. # (See https://git.io/v0rrR for implementation.) @@ -15,10 +14,9 @@ class Rack::Attack end PathNormalizer = if defined?(::ActionDispatch::Journey::Router::Utils) - # For Rails apps - ::ActionDispatch::Journey::Router::Utils - else - FallbackPathNormalizer - end - + # For Rails apps + ::ActionDispatch::Journey::Router::Utils + else + FallbackPathNormalizer + end end diff --git a/lib/rack/attack/safelist.rb b/lib/rack/attack/safelist.rb index 748d422..e548b0b 100644 --- a/lib/rack/attack/safelist.rb +++ b/lib/rack/attack/safelist.rb @@ -5,7 +5,6 @@ module Rack super @type = :safelist end - end end end diff --git a/lib/rack/attack/store_proxy.rb b/lib/rack/attack/store_proxy.rb index 5ce0f52..d8edddb 100644 --- a/lib/rack/attack/store_proxy.rb +++ b/lib/rack/attack/store_proxy.rb @@ -12,8 +12,8 @@ module Rack klass ? klass.new(client) : client end - private + def self.unwrap_active_support_stores(store) # ActiveSupport::Cache::RedisStore doesn't expose any way to set an expiry, # so use the raw Redis::Store instead. diff --git a/lib/rack/attack/store_proxy/dalli_proxy.rb b/lib/rack/attack/store_proxy/dalli_proxy.rb index 703f2d6..ccaa8bf 100644 --- a/lib/rack/attack/store_proxy/dalli_proxy.rb +++ b/lib/rack/attack/store_proxy/dalli_proxy.rb @@ -28,14 +28,14 @@ module Rack rescue Dalli::DalliError end - def write(key, value, options={}) + def write(key, value, options = {}) with do |client| client.set(key, value, options.fetch(:expires_in, 0), raw: true) end rescue Dalli::DalliError end - def increment(key, amount, options={}) + def increment(key, amount, options = {}) with do |client| client.incr(key, amount, options.fetch(:expires_in, 0), amount) end @@ -58,7 +58,6 @@ module Rack end end end - end end end diff --git a/lib/rack/attack/store_proxy/mem_cache_proxy.rb b/lib/rack/attack/store_proxy/mem_cache_proxy.rb index 098e048..bc23942 100644 --- a/lib/rack/attack/store_proxy/mem_cache_proxy.rb +++ b/lib/rack/attack/store_proxy/mem_cache_proxy.rb @@ -14,21 +14,21 @@ module Rack def read(key) # Second argument: reading raw value get(key, true) - rescue MemCache::MemCacheError + rescue MemCache::MemCacheError end - def write(key, value, options={}) + def write(key, value, options = {}) # Third argument: writing raw value set(key, value, options.fetch(:expires_in, 0), true) rescue MemCache::MemCacheError end - def increment(key, amount, options={}) + def increment(key, amount, options = {}) incr(key, amount) rescue MemCache::MemCacheError end - def delete(key, options={}) + def delete(key, options = {}) with do |client| client.delete(key) end @@ -44,7 +44,6 @@ module Rack end end end - end end end diff --git a/lib/rack/attack/store_proxy/redis_store_proxy.rb b/lib/rack/attack/store_proxy/redis_store_proxy.rb index 1a7c20a..9833c26 100644 --- a/lib/rack/attack/store_proxy/redis_store_proxy.rb +++ b/lib/rack/attack/store_proxy/redis_store_proxy.rb @@ -24,7 +24,7 @@ module Rack rescue Redis::BaseError end - def write(key, value, options={}) + def write(key, value, options = {}) if (expires_in = options[:expires_in]) setex(key, expires_in, value, raw: true) else @@ -33,7 +33,7 @@ module Rack rescue Redis::BaseError end - def increment(key, amount, options={}) + def increment(key, amount, options = {}) count = nil pipelined do @@ -45,7 +45,7 @@ module Rack rescue Redis::BaseError end - def delete(key, options={}) + def delete(key, options = {}) del(key) rescue Redis::BaseError end diff --git a/rack-attack.gemspec b/rack-attack.gemspec index 7ac83e8..6803d0a 100644 --- a/rack-attack.gemspec +++ b/rack-attack.gemspec @@ -1,4 +1,5 @@ # -*- encoding: utf-8 -*- + lib = File.expand_path('../lib/', __FILE__) $:.unshift lib unless $:.include?(lib) diff --git a/spec/acceptance/safelisting_ip_spec.rb b/spec/acceptance/safelisting_ip_spec.rb index 38faf72..eecd5b3 100644 --- a/spec/acceptance/safelisting_ip_spec.rb +++ b/spec/acceptance/safelisting_ip_spec.rb @@ -46,4 +46,3 @@ describe "Safelist an IP" do assert_equal :safelist, notification_type end end - diff --git a/spec/allow2ban_spec.rb b/spec/allow2ban_spec.rb index 9eeae9c..93564d2 100644 --- a/spec/allow2ban_spec.rb +++ b/spec/allow2ban_spec.rb @@ -7,10 +7,10 @@ describe 'Rack::Attack.Allow2Ban' do @findtime = 60 @bantime = 60 Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new - @f2b_options = {:bantime => @bantime, :findtime => @findtime, :maxretry => 2} + @f2b_options = { :bantime => @bantime, :findtime => @findtime, :maxretry => 2 } Rack::Attack.blocklist('pentest') do |req| - Rack::Attack::Allow2Ban.filter(req.ip, @f2b_options){req.query_string =~ /OMGHAX/} + Rack::Attack::Allow2Ban.filter(req.ip, @f2b_options) { req.query_string =~ /OMGHAX/ } end end @@ -31,7 +31,7 @@ describe 'Rack::Attack.Allow2Ban' do end it 'increases fail count' do - key = "rack::attack:#{Time.now.to_i/@findtime}:allow2ban:count:1.2.3.4" + key = "rack::attack:#{Time.now.to_i / @findtime}:allow2ban:count:1.2.3.4" @cache.store.read(key).must_equal 1 end @@ -53,7 +53,7 @@ describe 'Rack::Attack.Allow2Ban' do end it 'increases fail count' do - key = "rack::attack:#{Time.now.to_i/@findtime}:allow2ban:count:1.2.3.4" + key = "rack::attack:#{Time.now.to_i / @findtime}:allow2ban:count:1.2.3.4" @cache.store.read(key).must_equal 2 end @@ -89,7 +89,7 @@ describe 'Rack::Attack.Allow2Ban' do end it 'does not increase fail count' do - key = "rack::attack:#{Time.now.to_i/@findtime}:allow2ban:count:1.2.3.4" + key = "rack::attack:#{Time.now.to_i / @findtime}:allow2ban:count:1.2.3.4" @cache.store.read(key).must_equal 2 end @@ -109,7 +109,7 @@ describe 'Rack::Attack.Allow2Ban' do end it 'does not increase fail count' do - key = "rack::attack:#{Time.now.to_i/@findtime}:allow2ban:count:1.2.3.4" + key = "rack::attack:#{Time.now.to_i / @findtime}:allow2ban:count:1.2.3.4" @cache.store.read(key).must_equal 2 end diff --git a/spec/fail2ban_spec.rb b/spec/fail2ban_spec.rb index 9d4bcc1..e1fe7ef 100644 --- a/spec/fail2ban_spec.rb +++ b/spec/fail2ban_spec.rb @@ -7,10 +7,10 @@ describe 'Rack::Attack.Fail2Ban' do @findtime = 60 @bantime = 60 Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new - @f2b_options = {:bantime => @bantime, :findtime => @findtime, :maxretry => 2} + @f2b_options = { :bantime => @bantime, :findtime => @findtime, :maxretry => 2 } Rack::Attack.blocklist('pentest') do |req| - Rack::Attack::Fail2Ban.filter(req.ip, @f2b_options){req.query_string =~ /OMGHAX/} + Rack::Attack::Fail2Ban.filter(req.ip, @f2b_options) { req.query_string =~ /OMGHAX/ } end end @@ -31,7 +31,7 @@ describe 'Rack::Attack.Fail2Ban' do end it 'increases fail count' do - key = "rack::attack:#{Time.now.to_i/@findtime}:fail2ban:count:1.2.3.4" + key = "rack::attack:#{Time.now.to_i / @findtime}:fail2ban:count:1.2.3.4" @cache.store.read(key).must_equal 1 end @@ -53,7 +53,7 @@ describe 'Rack::Attack.Fail2Ban' do end it 'increases fail count' do - key = "rack::attack:#{Time.now.to_i/@findtime}:fail2ban:count:1.2.3.4" + key = "rack::attack:#{Time.now.to_i / @findtime}:fail2ban:count:1.2.3.4" @cache.store.read(key).must_equal 2 end @@ -75,7 +75,7 @@ describe 'Rack::Attack.Fail2Ban' do end it 'resets fail count' do - key = "rack::attack:#{Time.now.to_i/@findtime}:fail2ban:count:1.2.3.4" + key = "rack::attack:#{Time.now.to_i / @findtime}:fail2ban:count:1.2.3.4" assert_nil @cache.store.read(key) end @@ -110,7 +110,7 @@ describe 'Rack::Attack.Fail2Ban' do end it 'does not increase fail count' do - key = "rack::attack:#{Time.now.to_i/@findtime}:fail2ban:count:1.2.3.4" + key = "rack::attack:#{Time.now.to_i / @findtime}:fail2ban:count:1.2.3.4" @cache.store.read(key).must_equal 2 end @@ -130,7 +130,7 @@ describe 'Rack::Attack.Fail2Ban' do end it 'does not increase fail count' do - key = "rack::attack:#{Time.now.to_i/@findtime}:fail2ban:count:1.2.3.4" + key = "rack::attack:#{Time.now.to_i / @findtime}:fail2ban:count:1.2.3.4" @cache.store.read(key).must_equal 2 end diff --git a/spec/rack_attack_dalli_proxy_spec.rb b/spec/rack_attack_dalli_proxy_spec.rb index 50a2b9c..9065354 100644 --- a/spec/rack_attack_dalli_proxy_spec.rb +++ b/spec/rack_attack_dalli_proxy_spec.rb @@ -1,10 +1,8 @@ require_relative 'spec_helper' describe Rack::Attack::StoreProxy::DalliProxy do - it 'should stub Dalli::Client#with on older clients' do proxy = Rack::Attack::StoreProxy::DalliProxy.new(Class.new) proxy.with {} # will not raise an error end - end diff --git a/spec/rack_attack_spec.rb b/spec/rack_attack_spec.rb index 38733b2..3869b71 100644 --- a/spec/rack_attack_spec.rb +++ b/spec/rack_attack_spec.rb @@ -5,7 +5,7 @@ describe 'Rack::Attack' do describe 'normalizing paths' do before do - Rack::Attack.blocklist("banned_path") {|req| req.path == '/foo' } + Rack::Attack.blocklist("banned_path") { |req| req.path == '/foo' } end it 'blocks requests with trailing slash' do @@ -17,7 +17,7 @@ describe 'Rack::Attack' do describe 'blocklist' do before do @bad_ip = '1.2.3.4' - Rack::Attack.blocklist("ip #{@bad_ip}") {|req| req.ip == @bad_ip } + Rack::Attack.blocklist("ip #{@bad_ip}") { |req| req.ip == @bad_ip } end it('has a blocklist') { @@ -25,7 +25,7 @@ describe 'Rack::Attack' do } it('has a blacklist with a deprication warning') { - _, stderror = capture_io do + _, stderror = capture_io do Rack::Attack.blacklists.key?("ip #{@bad_ip}").must_equal true end assert_match "[DEPRECATION] 'Rack::Attack.blacklists' is deprecated. Please use 'blocklists' instead.", stderror @@ -50,13 +50,13 @@ describe 'Rack::Attack' do describe "and safelist" do before do @good_ua = 'GoodUA' - Rack::Attack.safelist("good ua") {|req| req.user_agent == @good_ua } + Rack::Attack.safelist("good ua") { |req| req.user_agent == @good_ua } end it('has a safelist') { Rack::Attack.safelists.key?("good ua") } it('has a whitelist with a deprication warning') { - _, stderror = capture_io do + _, stderror = capture_io do Rack::Attack.whitelists.key?("good ua") end assert_match "[DEPRECATION] 'Rack::Attack.whitelists' is deprecated. Please use 'safelists' instead.", stderror @@ -82,7 +82,7 @@ describe 'Rack::Attack' do end it 'should give a deprication warning for blacklisted_response' do - _, stderror = capture_io do + _, stderror = capture_io do Rack::Attack.blacklisted_response end assert_match "[DEPRECATION] 'Rack::Attack.blacklisted_response' is deprecated. Please use 'blocklisted_response' instead.", stderror diff --git a/spec/rack_attack_throttle_spec.rb b/spec/rack_attack_throttle_spec.rb index df6235f..8a0df19 100644 --- a/spec/rack_attack_throttle_spec.rb +++ b/spec/rack_attack_throttle_spec.rb @@ -15,7 +15,7 @@ describe 'Rack::Attack.throttle' do before { get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' } it 'should set the counter for one request' do - key = "rack::attack:#{Time.now.to_i/@period}:ip/sec:1.2.3.4" + key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" Rack::Attack.cache.store.read(key).must_equal 1 end @@ -37,7 +37,7 @@ describe 'Rack::Attack.throttle' do it 'should tag the env' do last_request.env['rack.attack.matched'].must_equal 'ip/sec' last_request.env['rack.attack.match_type'].must_equal :throttle - last_request.env['rack.attack.match_data'].must_equal({:count => 2, :limit => 1, :period => @period}) + last_request.env['rack.attack.match_data'].must_equal({ :count => 2, :limit => 1, :period => @period }) last_request.env['rack.attack.match_discriminator'].must_equal('1.2.3.4') end @@ -60,7 +60,7 @@ describe 'Rack::Attack.throttle with limit as proc' do before { get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' } it 'should set the counter for one request' do - key = "rack::attack:#{Time.now.to_i/@period}:ip/sec:1.2.3.4" + key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" Rack::Attack.cache.store.read(key).must_equal 1 end @@ -84,7 +84,7 @@ describe 'Rack::Attack.throttle with period as proc' do before { get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' } it 'should set the counter for one request' do - key = "rack::attack:#{Time.now.to_i/@period}:ip/sec:1.2.3.4" + key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" Rack::Attack.cache.store.read(key).must_equal 1 end @@ -108,7 +108,7 @@ describe 'Rack::Attack.throttle with block retuning nil' do before { get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' } it 'should not set the counter' do - key = "rack::attack:#{Time.now.to_i/@period}:ip/sec:1.2.3.4" + key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" assert_nil Rack::Attack.cache.store.read(key) end diff --git a/spec/rack_attack_track_spec.rb b/spec/rack_attack_track_spec.rb index cb2f2c9..8e0f9c6 100644 --- a/spec/rack_attack_track_spec.rb +++ b/spec/rack_attack_track_spec.rb @@ -16,7 +16,7 @@ describe 'Rack::Attack.track' do end before do - Rack::Attack.track("everything"){ |req| true } + Rack::Attack.track("everything") { |req| true } end it_allows_ok_requests @@ -31,7 +31,7 @@ describe 'Rack::Attack.track' do before do Counter.reset # A second track - Rack::Attack.track("homepage"){ |req| req.path == "/"} + Rack::Attack.track("homepage") { |req| req.path == "/" } ActiveSupport::Notifications.subscribe("rack.attack") do |*args| Counter.incr @@ -47,14 +47,14 @@ describe 'Rack::Attack.track' do describe "without limit and period options" do it "should assign the track filter to a Check instance" do - tracker = Rack::Attack.track("homepage") { |req| req.path == "/"} + tracker = Rack::Attack.track("homepage") { |req| req.path == "/" } tracker.filter.class.must_equal Rack::Attack::Check end end describe "with limit and period options" do it "should assign the track filter to a Throttle instance" do - tracker = Rack::Attack.track("homepage", :limit => 10, :period => 10) { |req| req.path == "/"} + tracker = Rack::Attack.track("homepage", :limit => 10, :period => 10) { |req| req.path == "/" } tracker.filter.class.must_equal Rack::Attack::Throttle end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 3473eba..7d62673 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -12,7 +12,7 @@ require "rack/attack" begin require 'pry' rescue LoadError - #nothing to do here + # nothing to do here end if RUBY_ENGINE == "ruby" @@ -20,7 +20,6 @@ if RUBY_ENGINE == "ruby" end class MiniTest::Spec - include Rack::Test::Methods before do @@ -43,7 +42,7 @@ class MiniTest::Spec use Rack::Attack use Rack::Lint - run lambda {|env| [200, {}, ['Hello World']]} + run lambda { |env| [200, {}, ['Hello World']] } }.to_app end