[Catalyst-dev] patch candidate to fix HTTPS-on-80/HTTP-on-443 handling

[Darren Duncan]

Given the context of my Catalyst-users post yesterday:

   http://lists.scsys.co.uk/pipermail/catalyst/2010-August/025730.html

At the end of this message is a WORKSFORME patch (what svn diff outputs) on the 
Catalyst-Runtime trunk for which I haven't yet made a test.

I welcome feedback on it.

This change breaks 4-5 C-R tests, though I haven't looked at those yet and the 
tests may be making false assumptions and need fixing themselves or my patch may 
need more work.

The make-test for MyApp that had the problems now runs properly and still passes 
its tests.

I intend to commit this patch myself (to set precedents and practice for being a 
more regular committer eventually) in a new branch, after I have come up with a 
test and looked at the failing tests, committing the new initially-failing test 
first.

Then a core committer can merge it to trunk when it is deemed to be suitable.

The failing test summary report is:

Test Summary Report
-------------------
t/aggregate/live_engine_request_headers.t                           (Wstat: 256 
Tests: 18 Failed: 1)
   Failed test:  7
   Non-zero exit status: 1
t/aggregate/unit_core_script_server.t                               (Wstat: 
65280 Tests: 0 Failed: 0)
   Non-zero exit status: 255
   Parse errors: No plan found in TAP output
t/author/http-server.t                                              (Wstat: 
15360 Tests: 0 Failed: 0)
   Non-zero exit status: 60
   Parse errors: Bad plan.  You planned 1 tests but ran 0.
t/dead_no_unknown_error.t                                           (Wstat: 
65280 Tests: 0 Failed: 0)
   Non-zero exit status: 255
   Parse errors: Bad plan.  You planned 1 tests but ran 0.
t/unit_core_methodattributes_method_metaclass_on_subclasses.t       (Wstat: 0 
Tests: 2 Failed: 0)
   TODO passed:   1
Files=131, Tests=2720, 252 wallclock secs ( 0.90 usr  0.59 sys + 210.54 cusr 
8.47 csys = 220.50 CPU)
Result: FAIL
Failed 4/131 test programs. 1/2720 subtests failed.
make: *** [test_dynamic] Error 255

I welcome feedback.

Thank you. -- Darren Duncan



Index: lib/Catalyst/Engine/HTTP.pm
===================================================================
--- lib/Catalyst/Engine/HTTP.pm	(revision 13513)
+++ lib/Catalyst/Engine/HTTP.pm	(working copy)
@@ -97,6 +97,20 @@
      *STDIN->blocking(1);
  };

+=head2 $self->prepare_connection($c)
+
+=cut
+
+after prepare_connection => sub {
+    my ( $self, $c ) = @_;
+    # This test is inappropriate for CGI/FastCGI, which should just look
+    # for $ENV{HTTPS} being 'ON', since port 443 may not be HTTPS here.
+    # But it might be appropriate for HTTP-proxy which may lack HTTPS header.
+    if ( $ENV{SERVER_PORT} == 443 ) {
+        $request->secure(1);
+    }
+};
+
  =head2 $self->prepare_read($c)

  =cut
Index: lib/Catalyst/Engine/CGI.pm
===================================================================
--- lib/Catalyst/Engine/CGI.pm	(revision 13513)
+++ lib/Catalyst/Engine/CGI.pm	(working copy)
@@ -118,10 +118,13 @@
      if ( $ENV{HTTPS} && uc( $ENV{HTTPS} ) eq 'ON' ) {
          $request->secure(1);
      }
+    # Declaring the request to be secure just because $ENV{SERVER_PORT} == 443
+    # is incorrect because port 443 may not always be running HTTPS; it might
+    # be running HTTP instead, for some reason.
+    # So testing that $ENV{HTTPS} exists and is 'ON' is the only reliable test
+    # for the CGI/FastCGI engines.
+    # But the HTTP-proxy engine might not see HTTPS and so it has the 443 test.

-    if ( $ENV{SERVER_PORT} == 443 ) {
-        $request->secure(1);
-    }
      binmode(STDOUT); # Ensure we are sending bytes.
  }

@@ -215,9 +218,15 @@
      my $uri_class = "URI::$scheme";

      # HTTP_HOST will include the port even if it's 80/443
-    $host =~ s/:(?:80|443)$//;
-
-    if ( $port !~ /^(?:80|443)$/ && $host !~ /:/ ) {
+    if ($host =~ s/:(.*)$//) {
+        $port = $1;  # since $ENV{SERVER_PORT} may be missing or wrong
+    }
+    if ($port == 80 and !$c->request->secure or $port == 443 and 
$c->request->secure) {
+        # The actual port matches the default port for the security status,
+        # so leave $host without a :$port.
+    }
+    else {
+        # The actual port doesn't match the default port for the security status.
          $host .= ":$port";
      }