Minor tweaks to prevent potential bugs, fixed how Forwarded is parsed…

…, updated README for clarity, and added a couple extra unit tests for IPv6 and general IP validation.
Kage · Jun 24, 2019 · 0490887 · 0490887
1 parent 6b9effb
commit 0490887
Show file tree

Hide file tree

Showing 5 changed files with 121 additions and 45 deletions.
diff --git a/README.pod b/README.pod
@@ -20,6 +20,7 @@ Version 0.03
     hide_headers    => 0,
   };
 
+  # Example of how you could verify expected functionality
   get '/test' => sub {
     my $c = shift;
     $c->render(json => {
@@ -40,11 +41,14 @@ transaction to override connecting user agent values only when the request comes
 from trusted upstream sources. You can specify multiple request headers where
 trusted upstream sources define the real user agent IP address or the real
 connection scheme, or disable either, and can hide the headers from the rest of
-the application if needed. This provides much of the same functionality as
-setting C<MOJO_REVERSE_PROXY=1>, but with more granular control over what
-headers to use and what upstream sources can send them. This is especially
-useful if your Mojolicious app is directly exposed to the internet, or if it
-sits behind multiple upstream proxies.
+the application if needed.
+
+This plugin provides much of the same functionality as setting
+C<MOJO_REVERSE_PROXY=1>, but with more granular control over what headers to
+use and what upstream sources can send them. This is especially useful if your
+Mojolicious app is directly exposed to the internet, or if it sits behind
+multiple upstream proxies. You should therefore ensure your application does
+not enable the default Mojolicious reverse proxy handler when using this plugin.
 
 This plugin supports parsing L<RFC 7239|http://tools.ietf.org/html/rfc7239>
 compliant C<Forwarded> headers, validates all IP addresses, and will
@@ -258,6 +262,12 @@ L<https://github.com/Kage/Mojolicious-Plugin-TrustedProxy>
 
 =over
 
+=item Hostnames not supported
+
+This plugin does not currently support hostnames or hostname resolution and
+there are no plans to implement this. If you have such a requirement, please
+feel free to submit a pull request.
+
 =item HTTP 'Forwarded' only partially supported
 
 Only partial support for RFC 7239 is currently implemented, but this should

diff --git a/lib/Mojolicious/Plugin/TrustedProxy.pm b/lib/Mojolicious/Plugin/TrustedProxy.pm
@@ -1,11 +1,12 @@
 package Mojolicious::Plugin::TrustedProxy;
+
+# https://github.com/Kage/Mojolicious-Plugin-TrustedProxy
+
 use Mojo::Base 'Mojolicious::Plugin';
 use Mojo::Util qw(trim monkey_patch);
+use Data::Validate::IP qw(is_ip is_ipv4_mapped_ipv6);
 use Net::CIDR::Lite;
 use Net::IP::Lite qw(ip_transform);
-use Data::Validate::IP qw(is_ip is_ipv4_mapped_ipv6);
-
-# https://github.com/Kage/Mojolicious-Plugin-TrustedProxy
 
 our $VERSION = '0.03';
 
@@ -64,12 +65,12 @@ sub register {
 
   # Register helper
   $app->helper(is_trusted_source => sub {
-    my $c = shift;
-    my $ip = shift || $c->tx->remote_proxy_address || $c->tx->remote_address;
+    my $c    = shift;
+    my $ip   = shift || $c->tx->remote_proxy_address || $c->tx->remote_address;
     my $cidr = $c->stash('trustedproxy.cidr');
     return undef unless
       is_ip($ip) && $cidr && $cidr->isa('Net::CIDR::Lite');
-    $ip = ip_transform($ip, {convert_to => 'ipv4'}) if (is_ipv4_mapped_ipv6($ip));
+    $ip = ip_transform($ip, {convert_to => 'ipv4'}) if is_ipv4_mapped_ipv6($ip);
     $c->app->log->debug(sprintf(
       '[%s] Testing if IP address "%s" is in trusted sources list',
       __PACKAGE__, $ip)) if DEBUG;
@@ -79,7 +80,7 @@ sub register {
   # Register hook
   $app->hook(around_dispatch => sub {
     my ($next, $c) = @_;
-    my $conf = $c->stash('trustedproxy.conf');
+    my $conf       = $c->stash('trustedproxy.conf');
     return $next->() unless defined $conf;
 
     # Validate that the upstream source IP is within the CIDR map
@@ -97,12 +98,12 @@ sub register {
         $ip = trim lc $ip;
         if (lc $header eq 'x-forwarded-for') {
           my @xff = split /\s*,\s*/, $ip;
-          $ip = $xff[0];
+          $ip = trim $xff[0];
         }
         $c->app->log->debug(sprintf(
           '[%s] Matched on IP header "%s" (value: "%s")',
           __PACKAGE__, $header, $ip)) if DEBUG;
-        $c->tx->remote_address($ip);
+        $c->tx->remote_address($ip) if is_ip($ip);
         $c->tx->remote_proxy_address($src_addr);
         last;
       }
@@ -112,7 +113,7 @@ sub register {
     foreach my $header (@{$conf->{scheme_headers}}) {
       if (my $scheme = $c->req->headers->header($header)) {
         $scheme = trim lc $scheme;
-        if (!!$scheme && grep { $scheme eq $_ } @{$conf->{https_values}}) {
+        if (!!$scheme && grep { $scheme eq lc $_ } @{$conf->{https_values}}) {
           $c->app->log->debug(sprintf(
             '[%s] Matched on HTTPS header "%s" (value: "%s")',
             __PACKAGE__, $header, $scheme)) if DEBUG;
@@ -132,12 +133,15 @@ sub register {
         my @pairs = map { split /\s*,\s*/, $_ } split ';', $fwd;
         my ($fwd_for, $fwd_by, $fwd_proto);
         my $ipv4_mask = qr/\d{1,3}.\d{1,3}.\d{1,3}.\d{1,3}/;
-        my $ipv6_mask = qr/\d{1,3}(?:\.\d{1,3}){0,2}/;
-        if ($pairs[0] =~ /(for|by)=($ipv4_mask|$ipv6_mask)/i) {
-          $fwd_for = trim($2 // $3) if lc $1 eq 'for';
-          $fwd_by  = trim($2 // $3) if lc $1 eq 'by';
-        } elsif ($pairs[0] =~ /proto=(https?)/i) {
-          $fwd_proto = trim $1;
+        my $ipv6_mask = qr/(([0-9a-fA-F]{0,4})([:|.])){2,7}([0-9a-fA-F]{0,4})/;
+        foreach my $param (@pairs) {
+          $param = trim $param;
+          if ($param =~ /(for|by)=($ipv4_mask|$ipv6_mask)/i) {
+            $fwd_for = $2 if lc $1 eq 'for';
+            $fwd_by  = $2 if lc $1 eq 'by';
+          } elsif ($param =~ /proto=(https?)/i) {
+            $fwd_proto = $1;
+          }
         }
         if ($fwd_for && is_ip($fwd_for)) {
           $c->app->log->debug(sprintf(
@@ -176,24 +180,6 @@ sub register {
 
 }
 
-#=begin html
-#
-#<a href="https://travis-ci.org/Kage/Mojolicious-Plugin-TrustedProxy">
-#<img src="https://travis-ci.org/Kage/Mojolicious-Plugin-TrustedProxy.svg?branch=master">
-#</a>
-#
-#=end html
-#
-#Code coverage:
-#
-#=begin html
-#
-#<a href='https://coveralls.io/github/Kage/Mojolicious-Plugin-TrustedProxy?branch=master'>
-#<img src='https://coveralls.io/repos/github/Kage/Mojolicious-Plugin-TrustedProxy/badge.svg?branch=master'>
-#</a>
-#
-#=end html
-
 1;
 __END__
 =head1 NAME
@@ -218,6 +204,7 @@ Version 0.03
     hide_headers    => 0,
   };
 
+  # Example of how you could verify expected functionality
   get '/test' => sub {
     my $c = shift;
     $c->render(json => {
@@ -238,11 +225,14 @@ transaction to override connecting user agent values only when the request comes
 from trusted upstream sources. You can specify multiple request headers where
 trusted upstream sources define the real user agent IP address or the real
 connection scheme, or disable either, and can hide the headers from the rest of
-the application if needed. This provides much of the same functionality as
-setting C<MOJO_REVERSE_PROXY=1>, but with more granular control over what
-headers to use and what upstream sources can send them. This is especially
-useful if your Mojolicious app is directly exposed to the internet, or if it
-sits behind multiple upstream proxies.
+the application if needed.
+
+This plugin provides much of the same functionality as setting
+C<MOJO_REVERSE_PROXY=1>, but with more granular control over what headers to
+use and what upstream sources can send them. This is especially useful if your
+Mojolicious app is directly exposed to the internet, or if it sits behind
+multiple upstream proxies. You should therefore ensure your application does
+not enable the default Mojolicious reverse proxy handler when using this plugin.
 
 This plugin supports parsing L<RFC 7239|http://tools.ietf.org/html/rfc7239>
 compliant C<Forwarded> headers, validates all IP addresses, and will
@@ -456,6 +446,12 @@ L<https://github.com/Kage/Mojolicious-Plugin-TrustedProxy>
 
 =over
 
+=item Hostnames not supported
+
+This plugin does not currently support hostnames or hostname resolution and
+there are no plans to implement this. If you have such a requirement, please
+feel free to submit a pull request.
+
 =item HTTP 'Forwarded' only partially supported
 
 Only partial support for RFC 7239 is currently implemented, but this should

diff --git a/t/01_ip_headers.t b/t/01_ip_headers.t
@@ -48,6 +48,24 @@ $t->get_ok('/ip' => {'X-Forwarded-For' => '1.1.1.1 , 2.2.2.2,3.3.3.3'})
     $TEST, $tid)
   );
 
+# Check IPv6 support
+$tid++;
+$tc += 3;
+$t->get_ok('/ip' => {'X-Forwarded-For' => 'fc01:c0ff:ee::'})
+  ->status_is(200)->content_is('fc01:c0ff:ee::', sprintf(
+    '[%s.%d] Assert from header X-Forwarded-For => fc01:c0ff:ee:: that tx->remote_address == fc01:c0ff:ee::',
+    $TEST, $tid)
+  );
+
+# Check bad IP value
+$tid++;
+$tc += 3;
+$t->get_ok('/ip' => {'X-Forwarded-For' => '123.456.789.000'})
+  ->status_is(200)->content_is('127.0.0.1', sprintf(
+    '[%s.%d] Assert from header X-Forwarded-For => 123.456.789.000 that tx->remote_address == 127.0.0.1',
+    $TEST, $tid)
+  );
+
 # Check remote_proxy_address
 $tid++;
 $tc += 3;

diff --git a/t/05_forwarded_header.t b/t/05_forwarded_header.t
@@ -30,7 +30,7 @@ $t->get_ok('/proxyip' => {'Forwarded' => 'by=1.1.1.1'})
     $TEST, $tid)
   );
 
-# Forwarded header protocol
+# Forwarded header protocol, also tests Forwarded override
 $tid++;
 $tc += 3;
 $t->get_ok('/scheme' => {'Forwarded' => 'proto=https', 'X-Forwarded-Proto' => 'http'})
@@ -39,4 +39,43 @@ $t->get_ok('/scheme' => {'Forwarded' => 'proto=https', 'X-Forwarded-Proto' => 'h
     $TEST, $tid)
   );
 
+# Forwarded with all values in one, plus IPv6 test
+my $fwd_params = {
+  for   => 'fc01:c0ff:ee::',
+  by    => 'fc01:c0de::',
+  proto => 'https',
+};
+
+$tc += 2;
+my $test = $t->get_ok('/all' => {
+  'Forwarded' => sprintf(
+    'for=%s ; by=%s;proto=%s',
+    $fwd_params->{for},
+    $fwd_params->{by},
+    $fwd_params->{proto}
+  ),
+})->status_is(200);
+
+# +- Test "for"
+$tid++;
+$tc++;
+$test->json_is('/ua_ip' => $fwd_params->{for}, sprintf(
+  '[%s.%d] from header Forwarded "for" that tx->remote_address == %s',
+  $TEST, $tid, $fwd_params->{for})
+);
+# +- Test "by"
+$tid++;
+$tc++;
+$test->json_is('/proxy_ip' => $fwd_params->{by}, sprintf(
+  '[%s.%d] from header Forwarded "by" that tx->remote_proxy_address == %s',
+  $TEST, $tid, $fwd_params->{by})
+);
+# +- Test "proto"
+$tid++;
+$tc++;
+$test->json_is('/scheme' => $fwd_params->{proto}, sprintf(
+  '[%s.%d] from header Forwarded "proto" that req->is_secure == %s',
+  $TEST, $tid, $fwd_params->{proto})
+);
+
 done_testing($tc);
diff --git a/t/lib/TestApp.pm b/t/lib/TestApp.pm
@@ -39,6 +39,19 @@ sub startup {
       $c->render(json => $c->req->headers->names);
     }
   );
+
+  # Returns all values (User agent IP, proxy IP, scheme, headers)
+  $r->get(
+    '/all' => sub {
+      my $c = shift;
+      $c->render(json => {
+        ua_ip    => $c->tx->remote_address,
+        proxy_ip => $c->tx->remote_proxy_address,
+        scheme   => $c->req->is_secure ? 'https' : 'http',
+        headers  => $c->req->headers->names,
+      });
+    }
+  );
 }
 
 1;