Incorrect detection of HTTPS 'on' in system/classes/site.php #288

Closed
lapsedtheorist opened this Issue Feb 6, 2012 · 0 comments

2 participants

@lapsedtheorist

I've found a problem with the line of code that decides on the protocol to use in Site::get_url('host') - the check against $_SERVER['HTTPS'] needs revising as follows:

diff --git a/classes/site.php b/classes/site.php
index 7048324..e36474e 100644
--- a/classes/site.php
+++ b/classes/site.php
@@ -144,7 +144,7 @@ class Site
                                if ( ( $port != 80 ) && ( $port != 443 ) && ( MultiByte::substr(
                                        $portpart = ':' . $port;
                                }
-                               if ( isset( $_SERVER['HTTPS'] ) && $_SERVER['HTTPS'] != 'off' ) 
+                               if ( isset( $_SERVER['HTTPS'] ) && $_SERVER['HTTPS'] == 'on' ) {
                                        $protocol = 'https';
                                }
                                $url = $protocol . '://' . $host . $portpart;

IIRC, the key 'HTTPS' can be set in $_SERVER even when HTTPS is off and the values it can contain are blank, false, null, 'off' and 'on'. Only the latter should result in $protocol = 'https';

Incidentally, I uncovered this issue from a fresh install of Nginx/1.1.12 on Debian Wheezy - the fastcgi_params file contains HTTPS irrespective of whether the server is using it or not, and if the server isn't using it then the value is blank.

Cheers,
Ben.

@mikelietz mikelietz added a commit to habari/system that referenced this issue Feb 6, 2012
@mikelietz mikelietz $_SERVER['HTTPS'] values that are not 'off' are not all 'on'. Accordi…
…ng to habari/habari#288 false and null are possible values as well. In both cases (in fact, in any case that is not 'on') the protocol should not be set to https. Thanks lapsedtheorist.
b3c39d2
@mikelietz mikelietz closed this Feb 6, 2012
@chrismeller chrismeller added a commit to habari/system that referenced this issue Nov 29, 2012
@chrismeller chrismeller Fix HTTPS detection, again.
I knew there was a reason habari/habari#288 would break things, I just couldn't remember it.

OpenStack sets HTTPS to 1, which is not equal to 'on'. So let's flip the logic back since we have a fairly inclusive list of things which indicate HTTPS is not being used and allow for just about anything else to indicate it is.

Our current list of possible conditions: off, on, empty string, false, null, and 0 / 1. There really should be a unit test for this...
d8fc70a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment