diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 08739a425b6..566c79be634 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -659,6 +659,10 @@ class ApplicationController < ActionController::Base render_to_string status: status, layout: layout, formats: [:html], template: '/exceptions/not_found' end + def is_asset_path + request.env['DISCOURSE_IS_ASSET_PATH'] = 1 + end + protected def render_post_json(post, add_raw = true) diff --git a/app/controllers/static_controller.rb b/app/controllers/static_controller.rb index 356214fbc2f..76a20858833 100644 --- a/app/controllers/static_controller.rb +++ b/app/controllers/static_controller.rb @@ -103,6 +103,8 @@ class StaticController < ApplicationController # instead we cache the favicon in redis and serve it out real quick with # a huge expiry, we also cache these assets in nginx so it bypassed if needed def favicon + is_asset_path + hijack do data = DistributedMemoizer.memoize(FAVICON + SiteSetting.favicon_url, 60 * 30) do begin @@ -138,16 +140,22 @@ class StaticController < ApplicationController end def brotli_asset + is_asset_path + serve_asset(".br") do response.headers["Content-Encoding"] = 'br' end end def cdn_asset + is_asset_path + serve_asset end def service_worker_asset + is_asset_path + respond_to do |format| format.js do # https://github.com/w3c/ServiceWorker/blob/master/explainer.md#updating-a-service-worker diff --git a/app/controllers/stylesheets_controller.rb b/app/controllers/stylesheets_controller.rb index 32b1f8be8e4..ef1837eab9a 100644 --- a/app/controllers/stylesheets_controller.rb +++ b/app/controllers/stylesheets_controller.rb @@ -6,6 +6,8 @@ class StylesheetsController < ApplicationController end def show + is_asset_path + show_resource end diff --git a/app/controllers/user_avatars_controller.rb b/app/controllers/user_avatars_controller.rb index dc868d48054..03e9e034712 100644 --- a/app/controllers/user_avatars_controller.rb +++ b/app/controllers/user_avatars_controller.rb @@ -33,6 +33,8 @@ class UserAvatarsController < ApplicationController end def show_proxy_letter + is_asset_path + if SiteSetting.external_system_avatars_url !~ /^\/letter_avatar_proxy/ raise Discourse::NotFound end @@ -56,6 +58,8 @@ class UserAvatarsController < ApplicationController end def show_letter + is_asset_path + params.require(:username) params.require(:version) params.require(:size) @@ -75,6 +79,8 @@ class UserAvatarsController < ApplicationController end def show + is_asset_path + # we need multisite support to keep a single origin pull for CDNs RailsMultisite::ConnectionManagement.with_hostname(params[:hostname]) do hijack do diff --git a/config/discourse_defaults.conf b/config/discourse_defaults.conf index fe949ee810a..6eb579aca88 100644 --- a/config/discourse_defaults.conf +++ b/config/discourse_defaults.conf @@ -178,6 +178,10 @@ max_admin_api_reqs_per_key_per_minute = 60 max_reqs_per_ip_per_minute = 200 max_reqs_per_ip_per_10_seconds = 50 + +# applies to asset type routes (avatars/css and so on) +max_asset_reqs_per_ip_per_10_seconds = 200 + # global rate limiter will simply warn if the limit is exceeded, can be warn+block, warn, block or none max_reqs_per_ip_mode = none diff --git a/lib/middleware/request_tracker.rb b/lib/middleware/request_tracker.rb index 543a5c60dc2..0ab468b482e 100644 --- a/lib/middleware/request_tracker.rb +++ b/lib/middleware/request_tracker.rb @@ -171,6 +171,16 @@ class Middleware::RequestTracker end result ensure + if (limiters = env['DISCOURSE_RATE_LIMITERS']) && env['DISCOURSE_IS_ASSET_PATH'] + limiters.each(&:rollback!) + env['DISCOURSE_ASSET_RATE_LIMITERS'].each do |limiter| + begin + limiter.performed! + rescue RateLimiter::LimitExceeded + # skip + end + end + end log_request_info(env, result, info) unless env["discourse.request_tracker.skip"] end @@ -213,16 +223,35 @@ class Middleware::RequestTracker global: true ) + limiter_assets10 = RateLimiter.new( + nil, + "global_ip_limit_10_assets_#{ip}", + GlobalSetting.max_asset_reqs_per_ip_per_10_seconds, + 10, + global: true + ) + + env['DISCOURSE_RATE_LIMITERS'] = [limiter10, limiter60] + env['DISCOURSE_ASSET_RATE_LIMITERS'] = [limiter_assets10] + + warn = GlobalSetting.max_reqs_per_ip_mode == "warn" || + GlobalSetting.max_reqs_per_ip_mode == "warn+block" + + if !limiter_assets10.can_perform? + if warn + Rails.logger.warn("Global asset IP rate limit exceeded for #{ip}: 10 second rate limit, uri: #{env["REQUEST_URI"]}") + end + + return !(GlobalSetting.max_reqs_per_ip_mode == "warn") + end + type = 10 begin limiter10.performed! type = 60 limiter60.performed! rescue RateLimiter::LimitExceeded - if ( - GlobalSetting.max_reqs_per_ip_mode == "warn" || - GlobalSetting.max_reqs_per_ip_mode == "warn+block" - ) + if warn Rails.logger.warn("Global IP rate limit exceeded for #{ip}: #{type} second rate limit, uri: #{env["REQUEST_URI"]}") !(GlobalSetting.max_reqs_per_ip_mode == "warn") else diff --git a/spec/components/middleware/request_tracker_spec.rb b/spec/components/middleware/request_tracker_spec.rb index d4455abbb4d..bc7b1d608ea 100644 --- a/spec/components/middleware/request_tracker_spec.rb +++ b/spec/components/middleware/request_tracker_spec.rb @@ -191,6 +191,28 @@ describe Middleware::RequestTracker do expect(status).to eq(200) end + it "allows assets for more requests" do + global_setting :max_reqs_per_ip_per_10_seconds, 1 + global_setting :max_reqs_per_ip_mode, 'block' + global_setting :max_asset_reqs_per_ip_per_10_seconds, 3 + + env1 = env("REMOTE_ADDR" => "1.1.1.1", "DISCOURSE_IS_ASSET_PATH" => 1) + + status, _ = middleware.call(env1) + expect(status).to eq(200) + status, _ = middleware.call(env1) + expect(status).to eq(200) + status, _ = middleware.call(env1) + expect(status).to eq(200) + status, _ = middleware.call(env1) + expect(status).to eq(429) + + env2 = env("REMOTE_ADDR" => "1.1.1.1") + + status, _ = middleware.call(env2) + expect(status).to eq(429) + end + it "does block if rate limiter is enabled" do global_setting :max_reqs_per_ip_per_10_seconds, 1 global_setting :max_reqs_per_ip_mode, 'block' @@ -199,13 +221,13 @@ describe Middleware::RequestTracker do env2 = env("REMOTE_ADDR" => "1.1.1.2") status, _ = middleware.call(env1) - status, _ = middleware.call(env1) + expect(status).to eq(200) + status, _ = middleware.call(env1) expect(status).to eq(429) status, _ = middleware.call(env2) expect(status).to eq(200) - end end