Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-28484

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: 2.8
    • Component/s: Libraries
    • 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

          Attachments

            Issue Links

              Activity

              poltawski Dan Poltawski created issue -
              poltawski Dan Poltawski made changes -
              Field Original Value New Value
              Assignee moodle.com [ moodle.com ] Dan Poltawski [ poltawski ]
              poltawski Dan Poltawski made changes -
              Status Open [ 1 ] Development in progress [ 3 ]
              Hide
              poltawski 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
              poltawski 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
              poltawski Dan Poltawski made changes -
              Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
              Peer reviewer skodak
              Hide
              poltawski Dan Poltawski added a comment -

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

              Show
              poltawski Dan Poltawski added a comment - An example of a use for this is in MDL-28486
              poltawski Dan Poltawski made changes -
              Link This issue blocks MDL-28486 [ MDL-28486 ]
              Hide
              skodak 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
              skodak 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
              skodak 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
              skodak 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...
              salvetore Michael de Raadt made changes -
              Fix Version/s STABLE backlog [ 10463 ]
              Labels triaged
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Hello Dan,

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

              Show
              rajeshtaneja Rajesh Taneja added a comment - Hello Dan, You might want to update all instances of https detection with added function.
              rajeshtaneja Rajesh Taneja made changes -
              Link This issue has a non-specific relationship to MDL-27364 [ MDL-27364 ]
              poltawski Dan Poltawski made changes -
              Status Waiting for peer review [ 10012 ] Development in progress [ 3 ]
              Hide
              dobedobedoh 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
              dobedobedoh 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
              skodak 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
              skodak Petr Skoda added a comment - hi 1/ it should go to weblib.php 2/ I guess it should return true on loginhttps pages too
              poltawski Dan Poltawski made changes -
              Link This issue has been marked as being related by MDL-38837 [ MDL-38837 ]
              poltawski Dan Poltawski made changes -
              Fix Version/s BACKEND [ 12582 ]
              poltawski Dan Poltawski made changes -
              Affects Version/s 2.5.2 [ 12664 ]
              Hide
              tonybutler 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
              tonybutler 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
              tonybutler Tony Butler made changes -
              Affects Version/s 2.7.1 [ 13550 ]
              Affects Version/s 2.6.4 [ 13551 ]
              Hide
              poltawski 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
              poltawski 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.
              poltawski Dan Poltawski made changes -
              Assignee Dan Poltawski [ poltawski ]
              poltawski Dan Poltawski made changes -
              Status Development in progress [ 3 ] Open [ 1 ]
              Hide
              tonybutler 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
              tonybutler 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
              tonybutler Tony Butler made changes -
              Assignee Tony Butler [ tonybutler ]
              Hide
              poltawski Dan Poltawski added a comment -

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

              Show
              poltawski Dan Poltawski added a comment - Actually, lets add John Okely here - as hes doing some related work in MDL-46269 - maybe this collides.
              poltawski Dan Poltawski made changes -
              Link This issue has a non-specific relationship to MDL-46269 [ MDL-46269 ]
              Hide
              tonybutler 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
              tonybutler 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
              kabalin 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
              kabalin 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.
              skodak Petr Skoda made changes -
              Peer reviewer Petr Skoda [ skodak ]
              Hide
              johno 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
              johno 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
              tonybutler Tony Butler added a comment -

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

              Show
              tonybutler Tony Butler added a comment - Requesting a peer review just to get some feedback on my approach to this.
              tonybutler Tony Butler made changes -
              Status Open [ 1 ] Waiting for peer review [ 10012 ]
              cibot CiBoT made changes -
              Labels patch triaged ci patch triaged
              Hide
              cibot 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 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
              johno John Okely made changes -
              Original Estimate 0 minutes [ 0 ]
              Remaining Estimate 0 minutes [ 0 ]
              Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
              Peer reviewer John Okely [ johno ]
              Hide
              johno 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
              johno 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
              johno John Okely made changes -
              Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
              johno John Okely made changes -
              Testing Instructions # Check that behat tests passed on nightly
              h4.For each of these scenarios
              # http site with no loginhttps
              # https site with no loginhttps
              # http site with loginhttps
              h4.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
              cibot CiBoT made changes -
              Labels ci patch triaged patch triaged
              johno John Okely made changes -
              Testing Instructions # Check that behat tests passed on nightly
              h4.For each of these scenarios
              # http site with no loginhttps
              # https site with no loginhttps
              # http site with loginhttps
              h4.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
              # Check that behat tests passed on nightly

              h4.For each of these scenarios
              # http site with no loginhttps
              # https site with no loginhttps
              # http site with loginhttps

              h4.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
              Hide
              kabalin 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
              kabalin 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
              johno 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
              johno 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
              tonybutler 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
              tonybutler 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
              tonybutler Tony Butler made changes -
              Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
              cibot CiBoT made changes -
              Labels patch triaged ci patch triaged
              Hide
              cibot CiBoT added a comment -
              Show
              cibot 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
              johno John Okely made changes -
              Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
              Hide
              johno 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
              johno 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.
              johno John Okely made changes -
              Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
              cibot CiBoT made changes -
              Labels ci patch triaged patch triaged
              Hide
              tonybutler 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
              tonybutler 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).
              tonybutler Tony Butler made changes -
              Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
              cibot CiBoT made changes -
              Labels patch triaged ci patch triaged
              Hide
              cibot CiBoT added a comment -
              Show
              cibot 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
              johno John Okely made changes -
              Status Waiting for peer review [ 10012 ] Waiting for integration review [ 10010 ]
              Hide
              cibot CiBoT added a comment -

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

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              cibot CiBoT made changes -
              Status Waiting for integration review [ 10010 ] Waiting for integration review [ 10010 ]
              Currently in integration Yes [ 10041 ]
              cibot CiBoT made changes -
              Labels ci patch triaged patch triaged
              cibot CiBoT made changes -
              Labels patch triaged ci patch triaged
              Hide
              tonybutler 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
              tonybutler 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
              skodak 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
              skodak 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
              skodak Petr Skoda added a comment -

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

              Show
              skodak Petr Skoda added a comment - Also the server global should not be probably modified in cli scripts.
              samhemelryk Sam Hemelryk made changes -
              Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
              Integrator Sam Hemelryk [ samhemelryk ]
              Hide
              samhemelryk 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
              samhemelryk 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
              samhemelryk Sam Hemelryk made changes -
              Status Integration review in progress [ 10004 ] Reopened [ 4 ]
              cibot CiBoT made changes -
              Labels ci patch triaged patch triaged
              cibot CiBoT made changes -
              Status Reopened [ 4 ] Reopened [ 4 ]
              Currently in integration Yes [ 10041 ]
              Hide
              cibot CiBoT added a comment -

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

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

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

              Change committed and rebased to latest master.

              Show
              tonybutler Tony Butler added a comment - - edited Thanks Sam, Petr. That makes a lot more sense. Change committed and rebased to latest master.
              tonybutler Tony Butler made changes -
              Labels patch triaged patch ready_for_integration triaged
              poltawski Dan Poltawski made changes -
              Status Reopened [ 4 ] Waiting for peer review [ 10012 ]
              poltawski Dan Poltawski made changes -
              Labels patch ready_for_integration triaged patch triaged
              cibot CiBoT made changes -
              Labels patch triaged ci patch triaged
              Hide
              cibot CiBoT added a comment -
              Show
              cibot 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
              johno John Okely made changes -
              Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
              Hide
              johno 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
              johno 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.
              johno John Okely made changes -
              Status Peer review in progress [ 10013 ] Waiting for integration review [ 10010 ]
              cibot CiBoT made changes -
              Status Waiting for integration review [ 10010 ] Waiting for integration review [ 10010 ]
              Integrator Sam Hemelryk [ samhemelryk ]
              Currently in integration Yes [ 10041 ]
              Hide
              cibot CiBoT added a comment -

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

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              cibot CiBoT made changes -
              Labels ci patch triaged patch triaged
              cibot CiBoT made changes -
              Labels patch triaged ci patch triaged
              damyon Damyon Wiese made changes -
              Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
              Integrator Damyon Wiese [ damyon ]
              Hide
              damyon 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 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.
              damyon Damyon Wiese made changes -
              Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
              Fix Version/s 2.8 [ 13150 ]
              Fix Version/s STABLE backlog [ 10463 ]
              Fix Version/s BACKEND [ 12582 ]
              damyon Damyon Wiese made changes -
              Labels ci patch triaged api_change ci dev_docs_required patch triaged
              stronk7 Eloy Lafuente (stronk7) made changes -
              Tester CiBoT [ cibot ]
              damyon Damyon Wiese made changes -
              Tester CiBoT [ cibot ] Andrew Nicols [ dobedobedoh ]
              dobedobedoh Andrew Nicols made changes -
              Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Everything seems to be A-OK to me

              Thanks all,

              Andrew

              Show
              dobedobedoh Andrew Nicols added a comment - Everything seems to be A-OK to me Thanks all, Andrew
              dobedobedoh Andrew Nicols made changes -
              Status Testing in progress [ 10011 ] Tested [ 10006 ]
              Hide
              poltawski 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
              poltawski 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
              poltawski Dan Poltawski made changes -
              Status Tested [ 10006 ] Closed [ 6 ]
              Resolution Fixed [ 1 ]
              Currently in integration Yes [ 10041 ]
              Integration date 10/Oct/14
              Hide
              danielneis Daniel Neis Araujo 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
              danielneis Daniel Neis Araujo 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
              damyon Damyon Wiese made changes -
              Link This issue caused a regression MDL-48050 [ MDL-48050 ]
              Hide
              damyon Damyon Wiese added a comment -

              Thanks for reporting this Daniel,

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

              Show
              damyon Damyon Wiese added a comment - Thanks for reporting this Daniel, I created MDL-48050 as a regression from this issue.
              johno John Okely made changes -
              Hide
              johno John Okely added a comment -

              This still needs some documentation although I'm not sure where. The dev docs themselves don't seem to have a lot on https etc.

              There's https://docs.moodle.org/dev/lib/weblib.php but I doubt anyone would go looking there for this info.

              Show
              johno John Okely added a comment - This still needs some documentation although I'm not sure where. The dev docs themselves don't seem to have a lot on https etc. There's https://docs.moodle.org/dev/lib/weblib.php but I doubt anyone would go looking there for this info.
              Hide
              johno John Okely added a comment -

              Documented usage on https://docs.moodle.org/dev/lib/weblib.php as I could not find a better place. It should now come up in search results for https and SSL on the dev wiki.

              Show
              johno John Okely added a comment - Documented usage on https://docs.moodle.org/dev/lib/weblib.php as I could not find a better place. It should now come up in search results for https and SSL on the dev wiki.
              johno John Okely made changes -
              Labels api_change ci dev_docs_required patch triaged api_change ci patch triaged
              johno John Okely made changes -
              Link This issue has been marked as being related by MDL-45539 [ MDL-45539 ]
              Hide
              johno John Okely added a comment -

              To all watchers there's some discussion about adding support for the X-Forwarded-Proto and Forwarded headers in MDL-45539. We could allow reverse proxies / load balancers to tell moodle about the original request. Which could help with a lot of ssl-related features of moodle.

              I'm no expert so if I've made any mistakes or bad assumptions in my documentation or the description on the issue feel free to change it.

              Show
              johno John Okely added a comment - To all watchers there's some discussion about adding support for the X-Forwarded-Proto and Forwarded headers in MDL-45539 . We could allow reverse proxies / load balancers to tell moodle about the original request. Which could help with a lot of ssl-related features of moodle. I'm no expert so if I've made any mistakes or bad assumptions in my documentation or the description on the issue feel free to change it.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    10/Nov/14