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

## Details

• Type: Bug
• Status: Closed
• Priority: Minor
• Resolution: Fixed
• Affects Version/s: 2.1, 2.5.2, 2.6.4, 2.7.1
• Fix Version/s:
• Component/s:
• Labels:
• 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

#### Complete these steps

1. Use a captcha in mod/feedback
3. Logout
4. Change user settings
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
• Pull Master Diff URL:

## Description

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

## 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

Fails against automated checks.

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

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

Fails against automated checks.

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

Show
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

Fails against automated checks.

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

Show
Hide

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

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

Fails against automated checks.

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

Show
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

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

• Assignee:
Tony Butler
Reporter:
Dan Poltawski
Peer reviewer:
John Okely
Integrator:
Damyon Wiese
Tester:
Andrew Nicols
Participants: