Moodle
  1. Moodle
  2. MDL-28484

We do not have a function to detect if we are being served on https

    Details

    • Testing Instructions:
      Hide
      1. Check that behat tests passed on nightly

      For each of these scenarios

      1. http site with no loginhttps
      2. https site with no loginhttps
      3. http site with loginhttps

      Complete these steps

      1. Use a captcha in mod/feedback
      2. Attempt to login
      3. Logout
      4. Change user settings
      5. Upload a file
      6. Insert a file that has already been uploaded
      7. Ensure images and previews work as expected
      Show
      Check that behat tests passed on nightly For each of these scenarios http site with no loginhttps https site with no loginhttps http site with loginhttps Complete these steps Use a captcha in mod/feedback Attempt to login Logout Change user settings Upload a file Insert a file that has already been uploaded Ensure images and previews work as expected
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_28_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-28484-master

      Description

      As we don't have a function, different places around Moodle are all doing a string comparison themselves.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Poltawski added a comment -

            Hi Petr,

            You are are the resident expert on these https issues, I was wondering what you thought about this?

            (I don't like the name of my moodlelib function and I also don't know if moodlelib is the right place for this).

            The use case is that in a plugin I want to embed some content from an external site - and convert the (external) url to https:// if moodle is served on https - to prevent 'insecure content' warnings in IE

            Show
            Dan Poltawski added a comment - Hi Petr, You are are the resident expert on these https issues, I was wondering what you thought about this? (I don't like the name of my moodlelib function and I also don't know if moodlelib is the right place for this). The use case is that in a plugin I want to embed some content from an external site - and convert the (external) url to https:// if moodle is served on https - to prevent 'insecure content' warnings in IE
            Hide
            Dan Poltawski added a comment -

            An example of a use for this is in MDL-28486

            Show
            Dan Poltawski added a comment - An example of a use for this is in MDL-28486
            Hide
            Petr Skoda added a comment -

            Thinking aloud - hmm, there are two alternative locations:
            1/ setuplib.php - it verifies the HTTP_HOST and sets up the FULLME, ME and friends
            2/ $PAGE->https_required() does something related, the problem is that the PAGE should not be used everywhere (I am not really sure about filelib.php now)

            The linked MDL->28486 should probably use something like PAGE->is_https() because it should probably deal with loginhttps too in case somebody uses youtube on the login/profile page.

            Show
            Petr Skoda added a comment - Thinking aloud - hmm, there are two alternative locations: 1/ setuplib.php - it verifies the HTTP_HOST and sets up the FULLME, ME and friends 2/ $PAGE->https_required() does something related, the problem is that the PAGE should not be used everywhere (I am not really sure about filelib.php now) The linked MDL->28486 should probably use something like PAGE->is_https() because it should probably deal with loginhttps too in case somebody uses youtube on the login/profile page.
            Hide
            Petr Skoda added a comment -

            Hmm, we should really get rid of the text caching we have in format_text and pass the PAGE as filter parameter instead. The plugins that need access to db could cache only the necessary information more effectively. There is a bug for this somewhere, I can not find it now, sorry...

            Show
            Petr Skoda added a comment - Hmm, we should really get rid of the text caching we have in format_text and pass the PAGE as filter parameter instead. The plugins that need access to db could cache only the necessary information more effectively. There is a bug for this somewhere, I can not find it now, sorry...
            Hide
            Rajesh Taneja added a comment -

            Hello Dan,

            You might want to update all instances of https detection with added function.

            Show
            Rajesh Taneja added a comment - Hello Dan, You might want to update all instances of https detection with added function.
            Hide
            Andrew Nicols added a comment -

            Petr:

            I'm looking at this again in combination with an updated patch for MDL-28486 (youtube/vimeo).

            I was pondering between:

            • lib/pagelib.php
            • lib/weblib.php
            • lib/setuplib.php

            With pagelib, I agree that it should be on a per-page basis. The difficulty here comes in making sure that we're not duplicating code or forgetting scenarios (e.g. checking that login, profile, and admin pages return the correct value based on loginhttps).

            I also considered using setuplib to determine the correct return value and adding a function to weblib – the checks for sslproxy and friends are all located in the initialise_fullme() function. It would be pretty simple to check the value of $rurl['scheme'] but it feels a touch wrong. Additionally, I'm not sure of the best place to store the value - $CFG feels wrong too.

            Have you got any thoughts on this?

            TIA,

            Andrew

            Show
            Andrew Nicols added a comment - Petr: I'm looking at this again in combination with an updated patch for MDL-28486 (youtube/vimeo). I was pondering between: lib/pagelib.php lib/weblib.php lib/setuplib.php With pagelib, I agree that it should be on a per-page basis. The difficulty here comes in making sure that we're not duplicating code or forgetting scenarios (e.g. checking that login, profile, and admin pages return the correct value based on loginhttps). I also considered using setuplib to determine the correct return value and adding a function to weblib – the checks for sslproxy and friends are all located in the initialise_fullme() function. It would be pretty simple to check the value of $rurl ['scheme'] but it feels a touch wrong. Additionally, I'm not sure of the best place to store the value - $CFG feels wrong too. Have you got any thoughts on this? TIA, Andrew
            Hide
            Petr Skoda added a comment -

            hi

            1/ it should go to weblib.php
            2/ I guess it should return true on loginhttps pages too

            Show
            Petr Skoda added a comment - hi 1/ it should go to weblib.php 2/ I guess it should return true on loginhttps pages too
            Hide
            Tony Butler added a comment - - edited

            Are there any plans to implement this soon?

            We just fell foul of an instance of (!isset($_SERVER['HTTPS']) || $_SERVER['HTTPS'] != "on") ? 'http' : 'https'; in mod_lti, which of course failed to detect that our reverse proxies serve HTTPS, because our web servers themselves don't. Took quite a while to figure out why our external tool wasn't working properly.

            Ruslan has just found another 22 instances of $_SERVER['HTTPS'].

            Cheers,
            Tony

            Show
            Tony Butler added a comment - - edited Are there any plans to implement this soon? We just fell foul of an instance of (!isset($_SERVER ['HTTPS'] ) || $_SERVER ['HTTPS'] != "on") ? 'http' : 'https'; in mod_lti, which of course failed to detect that our reverse proxies serve HTTPS, because our web servers themselves don't. Took quite a while to figure out why our external tool wasn't working properly. Ruslan has just found another 22 instances of $_SERVER ['HTTPS'] . Cheers, Tony
            Hide
            Dan Poltawski added a comment -

            Hi Tony,

            I'm not working on this t the moment - if you guys fancy taking this on it'd be great.

            Show
            Dan Poltawski added a comment - Hi Tony, I'm not working on this t the moment - if you guys fancy taking this on it'd be great.
            Hide
            Tony Butler added a comment - - edited

            I had a feeling that was coming Dan Poltawski. Yeah, I don't see any reason why not.

            So what's the best way forward? Is the method used in your patch still the HQ-recommended way to do it, or have things changed since then (or are they expected to)?

            Cheers,
            Tony

            Show
            Tony Butler added a comment - - edited I had a feeling that was coming Dan Poltawski . Yeah, I don't see any reason why not. So what's the best way forward? Is the method used in your patch still the HQ-recommended way to do it, or have things changed since then (or are they expected to)? Cheers, Tony
            Hide
            Dan Poltawski added a comment -

            Actually, lets add John Okely here - as hes doing some related work in MDL-46269 - maybe this collides.

            Show
            Dan Poltawski added a comment - Actually, lets add John Okely here - as hes doing some related work in MDL-46269 - maybe this collides.
            Hide
            Tony Butler added a comment -

            I've made a start, hopefully along the right track. I've replaced all the instances of individual detection that I could find apart from those in external libraries, installlib.php and setuplib.php (which presumably can't access $CFG->wwwroot because it hasn't necessarily been set up yet?).

            Is there anything we can do for external libraries? Ruslan mentioned that it was possible to override $_SERVER['HTTPS'].

            Show
            Tony Butler added a comment - I've made a start, hopefully along the right track. I've replaced all the instances of individual detection that I could find apart from those in external libraries, installlib.php and setuplib.php (which presumably can't access $CFG->wwwroot because it hasn't necessarily been set up yet?). Is there anything we can do for external libraries? Ruslan mentioned that it was possible to override $_SERVER ['HTTPS'] .
            Hide
            Ruslan Kabalin added a comment - - edited

            Overriding $_SERVER['HTTPS'] at the init stages should do the trick. I spotted some people are doing this to solve a problem when web server, located behind loadbalancer and ssl terminator, thinks it is http-only, while in fact the content is served using https. The common approach is to set "X-Forwarded-Proto" on loadbalancer to "https", and then track the header down in the application using the code similar to:

            if (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROTO'] == 'https') {
              $_SERVER['HTTPS'] = 'On';
            }
            

            In Moodle case, however, we know beforehand whether site is served using http or https, so there is no need to use "X-Forwarded-Proto", we can just rely on $CFG->wwwroot content and override $_SERVER['HTTPS'] to inform contrib libraries.

            Show
            Ruslan Kabalin added a comment - - edited Overriding $_SERVER ['HTTPS'] at the init stages should do the trick. I spotted some people are doing this to solve a problem when web server, located behind loadbalancer and ssl terminator, thinks it is http-only, while in fact the content is served using https. The common approach is to set "X-Forwarded-Proto" on loadbalancer to "https", and then track the header down in the application using the code similar to: if (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROTO'] == 'https') { $_SERVER['HTTPS'] = 'On'; } In Moodle case, however, we know beforehand whether site is served using http or https, so there is no need to use "X-Forwarded-Proto", we can just rely on $CFG->wwwroot content and override $_SERVER ['HTTPS'] to inform contrib libraries.
            Hide
            John Okely added a comment -

            Thanks for the heads up Dan I suspect this will conflict with MDL-42834 a lot once it's done so good to know about it.

            When working on MDL-42834 I was quite surprised to find that https was always checked with a string comparison great to hear this will get cleaned up!

            Show
            John Okely added a comment - Thanks for the heads up Dan I suspect this will conflict with MDL-42834 a lot once it's done so good to know about it. When working on MDL-42834 I was quite surprised to find that https was always checked with a string comparison great to hear this will get cleaned up!
            Hide
            Tony Butler added a comment -

            Requesting a peer review just to get some feedback on my approach to this.

            Show
            Tony Butler added a comment - Requesting a peer review just to get some feedback on my approach to this.
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-28484 using repository: https://github.com/lucisgit/moodle

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-28484 using repository: https://github.com/lucisgit/moodle master (branch: MDL-28484-master | CI Job ) Coding style problems found Testing instructions are missing. More information about this report
            Hide
            John Okely added a comment - - edited

            Thanks Tony, this is great!

            Do we consider

            (!isset($_SERVER['HTTPS']) || $_SERVER['HTTPS'] != "on")

            to be identical to

            strpos($CFG->wwwroot, 'https:')

            in all cases. If not, is this functionality we are prepared to change?

            Believe it or not, httpswwwroot will always be the same as wwwroot except for a login page on a http site when loginhttps is enabled. In which case httpswwwroot will start with https:// and wwwroot will start with http://. So you can make this change:

            -return (strpos($CFG->wwwroot, 'https://') === 0);
            +return (strpos($CFG->httpswwwroot, 'https://') === 0);
            

            And replace all these calls, too. (and any other you can find)

            filter/mathjaxloader/filter.php:            if (strpos($CFG->httpswwwroot, 'https:') === 0) {
            lib/outputrequirementslib.php:        if (strpos($CFG->httpswwwroot, 'https:') === 0) {
            lib/outputcomponents.php:            if (strpos($CFG->httpswwwroot, 'https:') === 0) {
            lib/form/recaptcha.php:        if (!empty($attributes['https']) or strpos($CFG->httpswwwroot, 'https:') === 0) {
            user/profile.php:    if (strpos($CFG->httpswwwroot, 'https:') === 0) {
            

            Then when we get rid of login https the function can be put back to $CFG->wwwroot. As a side benefit, this will make the total code change for deprecating loginhttps simpler, thanks

            Show
            John Okely added a comment - - edited Thanks Tony, this is great! Do we consider (!isset($_SERVER['HTTPS']) || $_SERVER['HTTPS'] != "on") to be identical to strpos($CFG->wwwroot, 'https:') in all cases. If not, is this functionality we are prepared to change? Believe it or not, httpswwwroot will always be the same as wwwroot except for a login page on a http site when loginhttps is enabled. In which case httpswwwroot will start with https:// and wwwroot will start with http:// . So you can make this change: -return (strpos($CFG->wwwroot, 'https://') === 0); +return (strpos($CFG->httpswwwroot, 'https://') === 0); And replace all these calls, too. (and any other you can find) filter/mathjaxloader/filter.php: if (strpos($CFG->httpswwwroot, 'https:') === 0) { lib/outputrequirementslib.php: if (strpos($CFG->httpswwwroot, 'https:') === 0) { lib/outputcomponents.php: if (strpos($CFG->httpswwwroot, 'https:') === 0) { lib/form/recaptcha.php: if (!empty($attributes['https']) or strpos($CFG->httpswwwroot, 'https:') === 0) { user/profile.php: if (strpos($CFG->httpswwwroot, 'https:') === 0) { Then when we get rid of login https the function can be put back to $CFG->wwwroot. As a side benefit, this will make the total code change for deprecating loginhttps simpler, thanks
            Hide
            Ruslan Kabalin added a comment -

            John, I think we may consider (!isset($_SERVER['HTTPS']) || $_SERVER['HTTPS'] != "on") to be identical to strpos($CFG->wwwroot, 'https:')
            in all cases. But there are some external libraries that are using $_SERVER['HTTPS'], thus, in addition to the change in the patch I suggest updating $_SERVER['HTTPS'] early based on is_https function output.

            Show
            Ruslan Kabalin added a comment - John, I think we may consider (!isset($_SERVER ['HTTPS'] ) || $_SERVER ['HTTPS'] != "on") to be identical to strpos($CFG->wwwroot, 'https:') in all cases. But there are some external libraries that are using $_SERVER ['HTTPS'] , thus, in addition to the change in the patch I suggest updating $_SERVER ['HTTPS'] early based on is_https function output.
            Hide
            John Okely added a comment - - edited

            Ah great.

            Since $_SERVER is supposed to be populated by the web server I think changing it directly might be solving the wrong problem, although I'm no expert in this area. Also, I'm pretty sure that $_SERVER will reset itself upon each request.

            Show
            John Okely added a comment - - edited Ah great. Since $_SERVER is supposed to be populated by the web server I think changing it directly might be solving the wrong problem, although I'm no expert in this area. Also, I'm pretty sure that $_SERVER will reset itself upon each request.
            Hide
            Tony Butler added a comment -

            Thanks for reviewing John, and for adding the testing instructions.

            I've updated the function as per your advice, and included the extra calls. I've also added Ruslan's $_SERVER['HTTPS'] override at the end of lib/setup.php (and it does seem to work). Thanks for that Ruslan.

            Cheers,
            Tony

            Show
            Tony Butler added a comment - Thanks for reviewing John, and for adding the testing instructions. I've updated the function as per your advice, and included the extra calls. I've also added Ruslan's $_SERVER ['HTTPS'] override at the end of lib/setup.php (and it does seem to work). Thanks for that Ruslan. Cheers, Tony
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-28484 using repository: https://github.com/lucisgit/moodle master (branch: MDL-28484-master | CI Job ) Coding style problems found More information about this report
            Hide
            John Okely added a comment -

            Thanks Tony. This looks good. If you could add anything to the testing instructions that needs testing that would be great, then it will be ready for integration review.

            Show
            John Okely added a comment - Thanks Tony. This looks good. If you could add anything to the testing instructions that needs testing that would be great, then it will be ready for integration review.
            Hide
            Tony Butler added a comment -

            Thanks John. It would be nice to add some way of testing the $_SERVER['HTTPS'] override, but it requires a fairly specific server environment (i.e. HTTP behind an HTTPS reverse proxy).

            Show
            Tony Butler added a comment - Thanks John. It would be nice to add some way of testing the $_SERVER ['HTTPS'] override, but it requires a fairly specific server environment (i.e. HTTP behind an HTTPS reverse proxy).
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-28484 using repository: https://github.com/lucisgit/moodle master (branch: MDL-28484-master | CI Job ) Coding style problems found More information about this report
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Tony Butler added a comment -

            Hi John,

            I've now tested the $_SERVER['HTTPS'] override in our production environment with the LTI tool that prompted me to take on this issue in the first place, and I can confirm that everything now works as expected.

            I've also rebased my diff branch to the latest master.

            Cheers,
            Tony

            Show
            Tony Butler added a comment - Hi John, I've now tested the $_SERVER ['HTTPS'] override in our production environment with the LTI tool that prompted me to take on this issue in the first place, and I can confirm that everything now works as expected. I've also rebased my diff branch to the latest master. Cheers, Tony
            Hide
            Petr Skoda added a comment -

            Hi, I believe the setup.php modification is incorrect - the _SERVER should be hacked only if sslproxy is enabled because without sslproxy it would not work anyway.

            Show
            Petr Skoda added a comment - Hi, I believe the setup.php modification is incorrect - the _SERVER should be hacked only if sslproxy is enabled because without sslproxy it would not work anyway.
            Hide
            Petr Skoda added a comment -

            Also the server global should not be probably modified in cli scripts.

            Show
            Petr Skoda added a comment - Also the server global should not be probably modified in cli scripts.
            Hide
            Sam Hemelryk added a comment -

            Hi Tony,

            First up thank you very much for working on this.

            I have just been speaking to Petr now about the two points that he noted and I am in agreement with both.
            _SERVER should only be hacked if sslproxy is enabled and its not a CLI script.
            On top of that while discussing it with him we identified that the _SERVER hack would perhaps be better located within initialise_fullme as it already has the required sslproxy handling and consolidating the ssl handling hacks would aid in us finding them in the future.

            Sending it back this week sorry so that these things can be looked into.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Tony, First up thank you very much for working on this. I have just been speaking to Petr now about the two points that he noted and I am in agreement with both. _SERVER should only be hacked if sslproxy is enabled and its not a CLI script. On top of that while discussing it with him we identified that the _SERVER hack would perhaps be better located within initialise_fullme as it already has the required sslproxy handling and consolidating the ssl handling hacks would aid in us finding them in the future. Sending it back this week sorry so that these things can be looked into. Cheers Sam
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Tony Butler added a comment - - edited

            Thanks Sam, Petr. That makes a lot more sense.

            Change committed and rebased to latest master.

            Show
            Tony Butler added a comment - - edited Thanks Sam, Petr. That makes a lot more sense. Change committed and rebased to latest master.
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-28484 using repository: https://github.com/lucisgit/moodle master (branch: MDL-28484-master | CI Job ) Coding style problems found More information about this report
            Hide
            John Okely added a comment -

            Looks good from what I can see, and the points from integration review have been addressed. Submitting for integration review.

            Show
            John Okely added a comment - Looks good from what I can see, and the points from integration review have been addressed. Submitting for integration review.
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Damyon Wiese added a comment -

            Thanks for finishing this Tony,

            Integrated to master. I added one more commit to update some comments that were suggesting the old strpos method of https detection.

            Show
            Damyon Wiese added a comment - Thanks for finishing this Tony, Integrated to master. I added one more commit to update some comments that were suggesting the old strpos method of https detection.
            Hide
            Andrew Nicols added a comment -

            Everything seems to be A-OK to me

            Thanks all,

            Andrew

            Show
            Andrew Nicols added a comment - Everything seems to be A-OK to me Thanks all, Andrew
            Hide
            Dan Poltawski added a comment -

            Thanks for your hard work, this change is now part of Moodle 2.8-beta.

            Why do we never have time to do it right, but always have time to do it over?
            --anonymous

            Show
            Dan Poltawski added a comment - Thanks for your hard work, this change is now part of Moodle 2.8-beta. Why do we never have time to do it right, but always have time to do it over? --anonymous
            Hide
            Daniel Neis added a comment -

            Hello,

            the change introduced here is causing problems to CAS authentication.
            Here at Universidade Federal de Santa Catarina, Brazil, we are using CFG->sslproxy with a HTTP server serving Moodle, both use Nginx and php-fpm.
            The happens at auth/cas/CAS/CAS/Client.php, when it tests the following:

            3503             if ( ($this->_isHttps() && $server_port!=443)
            3504                 || (!$this->_isHttps() && $server_port!=80)
            3505             ) {
            3506                 $server_url .= ':';
            3507                 $server_url .= $server_port;
            3508             }
            

            The "_isHttps()" returns true but the server_port is 80. Then it rewrites the URL adding the server_port value and it messes things. The final URL will be something like https://m28.moodle.ufsc.br:80/login

            Kind regards,
            Daniel

            Show
            Daniel Neis added a comment - Hello, the change introduced here is causing problems to CAS authentication. Here at Universidade Federal de Santa Catarina, Brazil, we are using CFG->sslproxy with a HTTP server serving Moodle, both use Nginx and php-fpm. The happens at auth/cas/CAS/CAS/Client.php, when it tests the following: 3503 if ( ($this->_isHttps() && $server_port!=443) 3504 || (!$this->_isHttps() && $server_port!=80) 3505 ) { 3506 $server_url .= ':'; 3507 $server_url .= $server_port; 3508 } The "_isHttps()" returns true but the server_port is 80. Then it rewrites the URL adding the server_port value and it messes things. The final URL will be something like https://m28.moodle.ufsc.br:80/login Kind regards, Daniel
            Hide
            Damyon Wiese added a comment -

            Thanks for reporting this Daniel,

            I created MDL-48050 as a regression from this issue.

            Show
            Damyon Wiese added a comment - Thanks for reporting this Daniel, I created MDL-48050 as a regression from this issue.

              People

              • Votes:
                2 Vote for this issue
                Watchers:
                10 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: