Geeklog-Core / geeklog

Geeklog - The Secure CMS.
https://www.geeklog.net
25 stars 19 forks source link

On Some Hosts COM_getCurrentURL does not return https when it is supposed to #1139

Closed eSilverStrike closed 1 year ago

eSilverStrike commented 1 year ago

COM_getCurrentURL uses the URL class and getCurrentURL function.

It looks like the following variables:

$_SERVER['SCRIPT_URI'] $_SERVER['REQUEST_SCHEME'] $_SERVER['SERVER_PORT']

may not be reliable depending on what is happening behind the scenes.

This means if you are using https it could be reported as http.

I am not sure why this exactly happens, but many other people report similar problems:

https://stackoverflow.com/questions/18008135/is-serverrequest-scheme-reliable

I was planning to move some Geeklog sites to Cloudways (a web host) and have run into this issue. One of the results is that I cannot save anything since the security tokens when saved to the database (in SEC_createToken), the URLs have the incorrect protocol (http instead of https).

So when the URLs is checked in the function SECINT_checkToken it fails since it uses $_SERVER['HTTP_REFERER'] to check the URL which has the correct protocol (https).

eSilverStrike commented 1 year ago

@mystralkk Can you take a look at url.class.php and getCurrentURL function.

On plan on replacing it with

public static function getCurrentURL($siteUrl)
{
    static $thisUrl;

    if ($thisUrl !== null) {
        return $thisUrl;
    }

    $thisUrl = '';

    $protocol = 'http://';
    if (isset($_SERVER['HTTPS'])) {
        if ('on' == strtolower( $_SERVER['HTTPS'])) {
            $protocol = 'https://';
        }

        if ('1' == $_SERVER['HTTPS']) {
            $protocol = 'https://';
        }
    } elseif (isset($_SERVER['SERVER_PORT']) && ('443' == $_SERVER['SERVER_PORT'])) {
        $protocol = 'https://';
    }       
    $thisUrl = $protocol . $_SERVER['HTTP_HOST'] . $_SERVER['REQUEST_URI'];

    return $thisUrl;
}

This should fix the current issue and is the recommend way I found to get the current URL for Apache web sites.

The problem is I know this doesn't work for IIS since IIS doesn't support REQUEST_URI and I have no way to test a solution for IIS.

What do you think?

eSilverStrike commented 1 year ago

@mystralkk I'll add an isset check to $_SERVER['REQUEST_URI']; and substitute it for $_SERVER['PHP_SELF']; for IIS I guess.

I see a few suggestions for this which should hopefully cover IIS

I also hope this works for when the site is installed in a folder off the domain and not the actual domain itself.

I'll add some comments as well so if any issues crop up we know where to look for some insight.

Any thoughts you have on this? (else I will push the changes)

mystralkk commented 1 year ago

Sorry, for late reply. Adding isset() and using $_SERVER['PHP_SELF'] seem to be good for me, though I guess we'll have to filter $_SERVER['PHP_SELF'] since this variable can be manipulated.

eSilverStrike commented 1 year ago

@mystralkk Nothing was filtered before in the original COM_getCurrentURL function.

Also, wouldn't the REQUEST_URI would also suffer from the same issue.

The problem is if we filter using something like htmlspecialchars it changes all the & to & in the URL which then messes up the comparison needed by the security tokens (the URLs do not match) so saving of posts etc... fail.

We ran into this issue before when COM_handle404 displays the URL on the page (a link was injecting a SVG) so I actually sanitized the URL before it was displayed within the COM_handle404 function.

Does it make sense to let the calling function handle the sanitizing of the return URL if needed? (Else how would you do it)

mystralkk commented 1 year ago

Does it make sense to let the calling function handle the sanitizing of the return URL if needed? (Else how would you do it)

I don't think it is good to leave the sanitizing of the return URL to the calling function, because the callers don't always handle URLs consistently. It would be better to create a new function like Url::cleanUrl(string $url): string.

eSilverStrike commented 1 year ago

Okay makes sense. I will add the function so the calling function can use it if it needs a sanitized URL.