mirror of
https://github.com/discourse/discourse.git
synced 2024-12-14 01:43:57 +08:00
SECURITY: Fix is_private_ip for RateLimiter to cover all cases (#12464)
The regular expression to detect private IP addresses did not always detect them successfully. Changed to use ruby's in-built IPAddr.new(ip_address).private? method instead which does the same thing but covers all cases.
This commit is contained in:
parent
7b283b5f21
commit
3ef594b1f1
|
@ -213,11 +213,9 @@ class Middleware::RequestTracker
|
||||||
log_request_info(env, result, info) unless !log_request || env["discourse.request_tracker.skip"]
|
log_request_info(env, result, info) unless !log_request || env["discourse.request_tracker.skip"]
|
||||||
end
|
end
|
||||||
|
|
||||||
PRIVATE_IP ||= /^(127\.)|(192\.168\.)|(10\.)|(172\.1[6-9]\.)|(172\.2[0-9]\.)|(172\.3[0-1]\.)|(::1$)|([fF][cCdD])/
|
|
||||||
|
|
||||||
def is_private_ip?(ip)
|
def is_private_ip?(ip)
|
||||||
ip = IPAddr.new(ip) rescue nil
|
ip = IPAddr.new(ip) rescue nil
|
||||||
!!(ip && ip.to_s.match?(PRIVATE_IP))
|
!!(ip && (ip.private? || ip.loopback?))
|
||||||
end
|
end
|
||||||
|
|
||||||
def rate_limit(request)
|
def rate_limit(request)
|
||||||
|
|
|
@ -181,13 +181,18 @@ describe Middleware::RequestTracker do
|
||||||
global_setting :max_reqs_per_ip_mode, 'warn+block'
|
global_setting :max_reqs_per_ip_mode, 'warn+block'
|
||||||
global_setting :max_reqs_rate_limit_on_private, true
|
global_setting :max_reqs_rate_limit_on_private, true
|
||||||
|
|
||||||
env1 = env("REMOTE_ADDR" => "127.0.0.2")
|
addresses = %w[127.1.2.3 127.0.0.2 192.168.1.2 10.0.1.2 172.16.9.8 172.19.1.2 172.20.9.8 172.29.1.2 172.30.9.8 172.31.1.2]
|
||||||
|
warn_count = 1
|
||||||
|
addresses.each do |addr|
|
||||||
|
env1 = env("REMOTE_ADDR" => addr)
|
||||||
|
|
||||||
status, _ = middleware.call(env1)
|
status, _ = middleware.call(env1)
|
||||||
status, _ = middleware.call(env1)
|
status, _ = middleware.call(env1)
|
||||||
|
|
||||||
expect(Rails.logger.warnings).to eq(1)
|
expect(Rails.logger.warnings).to eq(warn_count)
|
||||||
expect(status).to eq(429)
|
expect(status).to eq(429)
|
||||||
|
warn_count += 1
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
describe "register_ip_skipper" do
|
describe "register_ip_skipper" do
|
||||||
|
@ -223,13 +228,16 @@ describe Middleware::RequestTracker do
|
||||||
global_setting :max_reqs_per_ip_mode, 'warn+block'
|
global_setting :max_reqs_per_ip_mode, 'warn+block'
|
||||||
global_setting :max_reqs_rate_limit_on_private, false
|
global_setting :max_reqs_rate_limit_on_private, false
|
||||||
|
|
||||||
env1 = env("REMOTE_ADDR" => "127.0.3.1")
|
addresses = %w[127.1.2.3 127.0.3.1 192.168.1.2 10.0.1.2 172.16.9.8 172.19.1.2 172.20.9.8 172.29.1.2 172.30.9.8 172.31.1.2]
|
||||||
|
addresses.each do |addr|
|
||||||
|
env1 = env("REMOTE_ADDR" => addr)
|
||||||
|
|
||||||
status, _ = middleware.call(env1)
|
status, _ = middleware.call(env1)
|
||||||
status, _ = middleware.call(env1)
|
status, _ = middleware.call(env1)
|
||||||
|
|
||||||
expect(Rails.logger.warnings).to eq(0)
|
expect(Rails.logger.warnings).to eq(0)
|
||||||
expect(status).to eq(200)
|
expect(status).to eq(200)
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
it "does warn if rate limiter is enabled via warn+block" do
|
it "does warn if rate limiter is enabled via warn+block" do
|
||||||
|
|
Loading…
Reference in New Issue
Block a user