From 1e3058f38d8754428044eb3d5626ee54497dd039 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 8 Jul 2020 07:10:50 +0000 Subject: [PATCH] Bug 25950: Remove in X-Forwarded-For from proxy tests This patch removes the ip address in the X-Forwarded-For header from being tested against koha_trusted_proxies. Without this patch, REMOTE_ADDR will be set to null, if the ip address matches against koha_trusted_proxies. To Test: 1. Run the unit test t/Koha/Middleware/RealIP.t --- Koha/Middleware/RealIP.pm | 3 +- t/Koha/Middleware/RealIP.t | 152 +++++++++++++++++++++++++++++---------------- 2 files changed, 100 insertions(+), 55 deletions(-) diff --git a/Koha/Middleware/RealIP.pm b/Koha/Middleware/RealIP.pm index 88a843dbcf..fc9f280ff6 100644 --- a/Koha/Middleware/RealIP.pm +++ b/Koha/Middleware/RealIP.pm @@ -81,9 +81,10 @@ sub get_real_ip { my $trusted_proxies = get_trusted_proxies(); + #X-Forwarded-For: , , + my $real_ip = shift @forwarded_for; my @unconfirmed = ( @forwarded_for, $remote_addr ); - my $real_ip; while (my $addr = pop @unconfirmed) { my $has_matched = 0; foreach my $netmask (@$trusted_proxies) { diff --git a/t/Koha/Middleware/RealIP.t b/t/Koha/Middleware/RealIP.t index 34bc02af26..c60994b189 100644 --- a/t/Koha/Middleware/RealIP.t +++ b/t/Koha/Middleware/RealIP.t @@ -20,7 +20,7 @@ use strict; use warnings; -use Test::More tests => 13; +use Test::More tests => 11; use Test::Warn; use t::lib::Mocks; @@ -28,62 +28,106 @@ use_ok("Koha::Middleware::RealIP"); my ($remote_address,$x_forwarded_for_header,$address); -$remote_address = "1.1.1.1"; -$x_forwarded_for_header = ""; -$address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); -is($address,'1.1.1.1',"There is no X-Forwarded-For header, so just use the remote address"); - -$remote_address = "1.1.1.1"; -$x_forwarded_for_header = "2.2.2.2"; -$address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); -is($address,'1.1.1.1',"Don't trust 1.1.1.1 as a proxy, so use it as the remote address"); - -$remote_address = "1.1.1.1"; -$x_forwarded_for_header = "2.2.2.2"; -t::lib::Mocks::mock_config('koha_trusted_proxies', '1.1.1.1'); -$address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); -is($address,'2.2.2.2',"Trust proxy (1.1.1.1), so use the X-Forwarded-For header for the remote address"); - - -$remote_address = "1.1.1.1"; -$x_forwarded_for_header = "2.2.2.2,3.3.3.3"; -t::lib::Mocks::mock_config('koha_trusted_proxies', '1.1.1.1 3.3.3.3'); -$address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); -is($address,'2.2.2.2',"Trust multiple proxies (1.1.1.1 and 3.3.3.3), so use the X-Forwaded-For portion for the remote address"); - -$remote_address = "1.1.1.1"; -$x_forwarded_for_header = "2.2.2.2,3.3.3.3"; -t::lib::Mocks::mock_config('koha_trusted_proxies', 'bad configuration'); -warnings_are { +subtest "No X-Forwarded-For header" => sub { + plan tests => 1; + $remote_address = "1.1.1.1"; + $x_forwarded_for_header = ""; $address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); -} ["could not parse bad","could not parse configuration"],"Warn on misconfigured koha_trusted_proxies"; -is($address,'1.1.1.1',"koha_trusted_proxies is misconfigured so ignore the X-Forwarded-For header"); + is($address,'1.1.1.1',"There is no X-Forwarded-For header, so just use the remote address"); +}; + +subtest "No configuration" => sub { + plan tests => 1; + $remote_address = "2.2.2.2"; + $x_forwarded_for_header = "1.1.1.1"; + $address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); + is($address,'2.2.2.2',"No trusted proxies available, so don't trust 2.2.2.2 as a proxy, instead use it as the remote address"); +}; + +subtest "Bad configuration" => sub { + plan tests => 4; + $remote_address = "1.1.1.1"; + $x_forwarded_for_header = "2.2.2.2,3.3.3.3"; + t::lib::Mocks::mock_config('koha_trusted_proxies', 'bad configuration'); + warnings_are { + $address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); + } ["could not parse bad","could not parse configuration"],"Warn on misconfigured koha_trusted_proxies"; + is($address,'1.1.1.1',"koha_trusted_proxies is misconfigured so ignore the X-Forwarded-For header"); + + $remote_address = "1.1.1.1"; + $x_forwarded_for_header = "2.2.2.2"; + t::lib::Mocks::mock_config('koha_trusted_proxies', 'bad 1.1.1.1'); + warning_is { + $address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); + } "could not parse bad","Warn on partially misconfigured koha_trusted_proxies"; + is($address,'2.2.2.2',"koha_trusted_proxies contains an invalid value but still includes one correct value, which is relevant, so use X-Forwarded-For header"); +}; + +subtest "Fail proxy isn't trusted" => sub { + plan tests => 1; + $remote_address = "2.2.2.2"; + $x_forwarded_for_header = "1.1.1.1"; + t::lib::Mocks::mock_config('koha_trusted_proxies', '3.3.3.3'); + $address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); + is($address,'2.2.2.2',"The 2.2.2.2 proxy isn't in our trusted list, so use it as the remote address"); +}; + +subtest "The most recent proxy is trusted but the proxy before it is not trusted" => sub { + plan tests => 1; + $remote_address = "3.3.3.3"; + $x_forwarded_for_header = "1.1.1.1, 2.2.2.2"; + t::lib::Mocks::mock_config('koha_trusted_proxies', '3.3.3.3'); + $address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); + is($address,'2.2.2.2',"We trust 3.3.3.3, but we don't trust 2.2.2.2, so use 2.2.2.2 as the remote address"); +}; + +subtest "Success one proxy" => sub { + plan tests => 1; + $remote_address = "2.2.2.2"; + $x_forwarded_for_header = "1.1.1.1"; + t::lib::Mocks::mock_config('koha_trusted_proxies', '2.2.2.2'); + $address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); + is($address,'1.1.1.1',"Trust proxy (2.2.2.2), so use the client IP from the X-Forwarded-For header for the remote address"); +}; + +subtest "Success multiple proxies" => sub { + plan tests => 1; + $remote_address = "2.2.2.2"; + $x_forwarded_for_header = "1.1.1.1,3.3.3.3"; + t::lib::Mocks::mock_config('koha_trusted_proxies', '2.2.2.2 3.3.3.3'); + $address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); + is($address,'1.1.1.1',"Trust multiple proxies (2.2.2.2 and 3.3.3.3), so use the X-Forwaded-For portion for the remote address"); +}; + +subtest "Test alternative configuration styles" => sub { + plan tests => 3; + $remote_address = "2.2.2.2"; + $x_forwarded_for_header = "1.1.1.1"; + t::lib::Mocks::mock_config('koha_trusted_proxies', '2.2.2.0/24'); + $address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); + is($address,'1.1.1.1',"Trust proxy (2.2.2.2) using CIDR notation, so use the X-Forwarded-For header for the remote address"); -$remote_address = "1.1.1.1"; -$x_forwarded_for_header = "2.2.2.2"; -t::lib::Mocks::mock_config('koha_trusted_proxies', 'bad 1.1.1.1'); -warning_is { + $remote_address = "2.2.2.2"; + $x_forwarded_for_header = "1.1.1.1"; + t::lib::Mocks::mock_config('koha_trusted_proxies', '2.2.2'); $address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); -} "could not parse bad","Warn on partially misconfigured koha_trusted_proxies"; -is($address,'2.2.2.2',"koha_trusted_proxies contains an invalid value but still includes one correct value, which is relevant, so use X-Forwarded-For header"); - -$remote_address = "1.1.1.1"; -$x_forwarded_for_header = "2.2.2.2"; -t::lib::Mocks::mock_config('koha_trusted_proxies', '1.1.1.0/24'); -$address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); -is($address,'2.2.2.2',"Trust proxy (1.1.1.1) using CIDR notation, so use the X-Forwarded-For header for the remote address"); - -$remote_address = "1.1.1.1"; -$x_forwarded_for_header = "2.2.2.2"; -t::lib::Mocks::mock_config('koha_trusted_proxies', '1.1.1'); -$address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); -is($address,'2.2.2.2',"Trust proxy (1.1.1.1) using abbreviated notation, so use the X-Forwarded-For header for the remote address"); - -$remote_address = "1.1.1.1"; -$x_forwarded_for_header = "2.2.2.2"; -t::lib::Mocks::mock_config('koha_trusted_proxies', '1.1.1.0:255.255.255.0'); -$address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); -is($address,'2.2.2.2',"Trust proxy (1.1.1.1) using an IP address and netmask separated by a colon, so use the X-Forwarded-For header for the remote address"); + is($address,'1.1.1.1',"Trust proxy (2.2.2.2) using abbreviated notation, so use the X-Forwarded-For header for the remote address"); + + $remote_address = "2.2.2.2"; + $x_forwarded_for_header = "1.1.1.1"; + t::lib::Mocks::mock_config('koha_trusted_proxies', '2.2.2.0:255.255.255.0'); + $address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); + is($address,'1.1.1.1',"Trust proxy (2.2.2.2) using an IP address and netmask separated by a colon, so use the X-Forwarded-For header for the remote address"); +}; + +subtest "Client IP is properly processed even if it is in koha_trusted_proxies" => sub { + $remote_address = "1.1.1.2"; + $x_forwarded_for_header = "1.1.1.1"; + t::lib::Mocks::mock_config('koha_trusted_proxies', '1.1.1.0/24'); + $address = Koha::Middleware::RealIP::get_real_ip( $remote_address, $x_forwarded_for_header ); + is($address,'1.1.1.1',"The X-Forwarded-For value matches the koha_trusted_proxies, but there is no proxy specified"); + done_testing(1); +}; subtest "IPv6 support" => sub { my ($remote_address,$x_forwarded_for_header,$address); -- 2.11.0