in reply to mod-perl2 re-using $ENV{REMOTE_ADDR}

As we don't see the data going in, this is going to be hard to debug.

Please add some code that outputs the parameters of the run, that is, output $ENV{HTTP_X_FORWARDED_FOR} and $ENV{REMOTE_ADDR}. Maybe it depends on the input data you get.

Also, while I am a fan of splice, the unshift keyword is a more clear function to use in that place.

Replies are listed 'Best First'.
Re^2: mod-perl2 re-using $ENV{REMOTE_ADDR}
by Bod (Parson) on May 01, 2023 at 22:27 UTC
    more clear function to use in that place

    Yes - the splice took a little working out...

    I feel that writing it like this (untested) would be the clearest 

    @IPs='';
if (defined $ENV{HTTP_X_FORWARDED_FOR}) {
  @IPs=split /,/, $ENV{HTTP_X_FORWARDED_FOR};
} else {
  @IPs=split /,/, $ENV{REMOTE_ADDR};
}

    [download]
    I also find the semicolon after the if block a bit strange.

[reply]
[d/l]
[select]
      I would write that: 
      @IPs=split /,/, $ENV{HTTP_X_FORWARDED_FOR} // $ENV{REMOTE_ADDR};

      [download]
      as it is both shorter and clearer in its intent, and doesn't pointlessly set @IPs only to immediately re-set it. For pre-5.10 you'd have to use a conditional operator, which is left as an exercise for the reader.
[reply]
[d/l]
[select]
        Or a list slice
[reply]
      Very much appreciate all the tips!

      We had an issue with some transparent proxies (the URL is served as http://), where those sometimes add HTTP_X_FORWARDED_FOR but then detail internal IPs. We subsequently may still wish to refer to the remote_addr if all the IPs in the http_x_forwarded_for list are all not valid. The following appears to be working perfectly now:
      use Data::Validate::IP qw(is_ip is_public_ip is_loopback_ip);

my $ip;
my @IPs;

if (defined $ENV{HTTP_X_FORWARDED_FOR}) {
  @IPs=split /,/, $ENV{HTTP_X_FORWARDED_FOR};
};
unshift (@IPs, $ENV{REMOTE_ADDR});
foreach $check (@IPs) {
  $check =~ s/^\s*(.*?)\s*$/\1/g;
  if (is_ip($check)) {
    if (defined $ip) {
      if ( (not is_public_ip($check))
        or (is_loopback_ip($check))
        or ($check eq "41.79.21.90") ) {
        $check = $ip;
      }
    }
    $ip = $check;
  }
}

print "Content-type: text/html\nCache-Control: max-age=0,no-cache,no-s
+tore,post-check=0,pre-check=0\n\n<html><head><title>Current IP Check<
+/title></head><body>Current IP Address: $ip</body></html>";

      [download]
[reply]
[d/l]

        I'm glad you have something that appears to be working for you. However I find the logic somewhat confusing, and somewhat perverse.

        I think the intention is to find a valid external IP, or failing that to fall back to any valid internal IP. If that is indeed the intention, I think it could be expressed more clearly with something like:

        my($valid_ip, $valid_external);
foreach my $check (@IPs) {
  # clean it up
  $check =~ s/^\s*(.*?)\s*$/\1/;

  # skip if not a valid IP
  next unless is_ip($check);
  # overwrite with the last IP we saw that's valid
  $valid_ip = $check;

  # skip unless it's an external IP
  next if (not is_public_ip($check))
      or (is_loopback_ip($check))
      or ($check eq "41.79.21.90");
  # overwrite with the last valid IP that is external
  $valid_external = $check;
}
# choose last external IP if we found one, else last valid IP
my $ip = $valid_external // $valid_ip;

        [download]

        Your current code favours the last external IP rather than the first one, and the last internal IP rather than the first one, but I cannot tell if either of those choices are intentional. My proposed code honours that preference, but if you actually want the _first_ external IP from the list you could add last; at the end of the loop to short-circuit it; if you want the first internal IP from the list, you can replace $valid_ip = $check with $valid_ip //= $check.

        Side-note: the //g option on the cleanup regexp serves no useful purpose, so I removed it.

        Hope this helps.

[reply]
[d/l]
[select]
Re^2: mod-perl2 re-using $ENV{REMOTE_ADDR}
by bbs2web (Acolyte) on May 01, 2023 at 14:07 UTC
    Thank you for your feedback, searching for 'splice 0,0 v unshift' appears to show that unshift is twice as performant as splice. This really isn't a large array though but if it's easier to read and faster in the process then why not... ;)


    It appears I wasn't initialising $ip, when I added 'undef $ip' it now works as expected:

    Apache's configuration stanza: 
    ScriptAlias /cgi-bin/ /var/www/perl-cgi/
PerlSwitches -wT
<Directory /var/www/perl-cgi>
        SetHandler perl-script
        PerlHandler ModPerl::Registry
        PerlSendHeader On
        AllowOverride None
        Options +ExecCGI -MultiViews
</Directory>

    [download]

    And the script where I now undefine the $ip variable at start:
    use Data::Validate::IP qw(is_ip is_public_ip is_loopback_ip);
@IPs='';
undef $ip;
if (defined $ENV{HTTP_X_FORWARDED_FOR}) {
  @IPs=split /,/, $ENV{HTTP_X_FORWARDED_FOR};
};
unshift (@IPs, $ENV{REMOTE_ADDR});
foreach $check (@IPs) {
  $check =~ s/^\s*(.*?)\s*$/\1/g;
  if (is_ip($check)) {
    if (defined $ip) {
      if (not is_public_ip($check)) { $check = $ip; };
      if (is_loopback_ip($check)) { $check = $ip; };
      if ($check eq "41.79.21.90") { $check = $ip; };
    };
    $ip = $check;
  };
};
print "Content-type: text/html\nCache-Control: max-age=0,no-cache,no-s
+tore,post-check=0,pre-check=0\n\n<html><head><title>Current IP Check<
+/title></head><body>Current IP Address: $ip</body></html>";

    [download]
[reply]
[d/l]
[select]

      Making the variables lexical variables would likely also help:

      use strict;

my @IPs;
my $ip;

      [download]
[reply]
[d/l]

In Section Seekers of Perl Wisdom