Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[2.x] Need to persist non-standard ports in site URL #16428

Closed
dimasites opened this issue May 6, 2023 · 10 comments
Closed

[2.x] Need to persist non-standard ports in site URL #16428

dimasites opened this issue May 6, 2023 · 10 comments
Labels
bug The issue in the code or project, which should be addressed.

Comments

@dimasites
Copy link
Contributor

dimasites commented May 6, 2023 •
edited
Loading

Bug report

Summary

Currently, when using a non-standard port for https (any one other than 443), the port gets stripped from the site's URL, making it impossible to properly navigate the site. Only the standard ports of 80 and 443 should be stripped. it is not fully resolved in 2.8.2 in this PR and ports like :44333 still gain errors

Step to reproduce

Configure the testing site (with site.local address for example) on :44333 port, login and click Preview site from naiin menu , or View from any resource admin page. You see site.local33/ in the broken url

Observed behavior

in sreenshot below, port :44333 MODX cut ":443" and "33" part breaks the links:
Still not correct working with non-standart ports

Expected behavior

Port :43333 and other similar must remain in the URL unchanged

Environment

MODX 2.8.5 version, apache/nginx LITESPEED/1.7.16
@dimasites dimasites added the bug The issue in the code or project, which should be addressed. label May 6, 2023
@dimasites dimasites mentioned this issue May 6, 2023
Merged
@JoshuaLuckers
Copy link
Contributor

Thanks for taking time to report this issue. Is this a clean install or an upgrade? Can you check the core/config.inc.php file and check if the port is defined correctly there?

@dimasites
Copy link
Contributor Author

dimasites commented May 7, 2023 via email •
edited
Loading

@JoshuaLuckers
Copy link
Contributor

Thanks for your answer, what's on line 67 of your config file?
It should be this:

$http_host .= in_array($_SERVER['SERVER_PORT'], [80, 443]) ? '' : ':' . $_SERVER['SERVER_PORT'];

@dimasites
Copy link
Contributor Author

Sorry for timeout... In my case, it is line 69, but i see something similar. Here is part of my core/config.inc.php file:
image

From installation date, i edited only config paths and db connect params... But with this config, bug reported above is reproduced

@dimasites
Copy link
Contributor Author

And in line 22:
$https_port = '443';

@opengeek
Copy link
Member

opengeek commented May 8, 2023 •
edited
Loading

It sounds like your webserver is not reporting the proper port to PHP. The result suggests it is in fact reporting that the port is 443, so the code is attempting to remove that from the http_host, but fails because the port is actually 44333 (which leaves the 33 without a colon preceding it). If you can get your server to properly report that $_SERVER['SERVER_PORT'] is 44333 and set your https_port configuration to 44333, everything should work properly.

@JoshuaLuckers
Copy link
Contributor

@opengeek I'm looking at the code but $https_port in the config file seems to be hardcoded to 443, unless it's in a $_POST value with key httpsport (but I can't find any reference to it in the code in the setup folder).

revolution/setup/includes/config/modconfigreader.class.php

Line 72 in 8b533fb

$this->config['https_port'] = isset($_POST['httpsport']) ? $_POST['httpsport'] : 443;

@opengeek
Copy link
Member

opengeek commented May 9, 2023

@JoshuaLuckers — then it is missing from the setup. It exists in the CLI setup config XML file, but doesn't look like the UI got a form field for it.

@dimasites dimasites mentioned this issue Jul 1, 2023
Merged
@dimasites
Copy link
Contributor Author

@opengeek hi, Sorry for the long answer. I make tests and prepare PR #16455 draft

he result suggests it is in fact reporting that the port is 443

You are right, my server gives port 443 (because it works on such a port, however, my configuration uses port forwarding to a private network) and nevertheless, the modx frontend works without problems (port 44333) the MODX backend is saved correctly too (mostly).

If you can get your server to properly report that $_SERVER['SERVER_PORT'] is 44333 and set your https_port configuration to 44333, everything should work properly.

In my case: port 44333 is port-mapping to the private local network, and inside this lan, site working on :443 and but outside - on :44333. In this case i should not change port in config.inc.php because this will lead to a breakdown of modx operation inside the network, where now :443

I did a lot of tests and found a solution PR #16455 (it is ready, but now draft, beacuse i cant solve problem with git-detection for only changed lines in first commit 2nd commint with config is well-tested by me, and 3rd commits is a port of this solution for installer)

opengeek pushed a commit that referenced this issue Mar 22, 2024
@dimasites
Fix issue working with non-standard ports (#16455)
1371d06 
### What does it do?
I changed logic of modx working with non-standard ports because old
logic in some cases works not correct: port :44333 MODX cut ":443" and
"33" part breaks the links adresses. See #16428 for details.

Relying on here
[this](https://stackoverflow.com/questions/4504831/serverhttp-host-contains-port-number-too
) and
[this](https://stackoverflow.com/questions/2297403/what-is-the-difference-between-http-host-and-server-name-in-php
) I suggest using the answers in stackoverflow to check the port not
`$_SERVER['SERVER_PORT']` but `parse_url($_SERVER['HTTP_HOST'],
PHP_URL_PORT)` because it works more reliably and correctly, as can be
seen from my tests.

### Why is it needed?
Currently, when using a non-standard port for https (any one other than
443) MODX modifies request URL with mistakes, and redirect users's
browser to wrong url.

See my explaination on awesome handmade infographic :) image:
![explaination for pull request
v2](https://github.com/modxcms/revolution/assets/5102558/14a7cc38-ea43-4475-8b5e-6baf6f58c3e7)

### How to test
I have prepared a test script:
<details><summary>See code:
test_modx_working_with_http_ports.php</summary>

```php

var_dump(parse_url($_SERVER['HTTP_HOST'], PHP_URL_PORT)); echo ' ← parsed port from HTTP_HOST <br>';
var_dump($_SERVER['SERVER_PORT']); echo ' ← SERVER_PORT <br>';

$http_port = parse_url($_SERVER['HTTP_HOST'], PHP_URL_PORT) ?: $_SERVER['SERVER_PORT'];
var_dump($_SERVER['SERVER_PORT']); echo ' ← http_port for usage in fixed config <br>';

$http_host = array_key_exists('HTTP_HOST', $_SERVER) ? htmlspecialchars($_SERVER['HTTP_HOST'], ENT_QUOTES) : 'test-port.ltd';

//this is for test logic with non-standart port without server changes, COMMENT IR FOR REAL SERVER TEST
$http_host = 'test-port.ltd:44333';

echo '<br>';
var_dump($http_host); echo ' ← http_host (old logic)<br>';

$http_host_new_logic = parse_url($http_host, PHP_URL_HOST); 
var_dump($http_host_new_logic); echo ' ← http_host (new logic)<br>';


/*old logic */
if ($_SERVER['SERVER_PORT'] !== 80) {
	
	$orig_http_host = str_replace(':' . $_SERVER['SERVER_PORT'], '', $http_host);
	$fixed_http_host = str_replace(':' . $http_port, '', $http_host);
	$fixed_http_host_new_logic = str_replace(':' . $http_port, '', $http_host_new_logic);
	
	$http_host = str_replace(':' . $_SERVER['SERVER_PORT'], '', $http_host);
}
echo '<br>';
var_dump($orig_http_host); echo ' ← orig_http_host after check port and clean host (old logic)<br>';
var_dump($fixed_http_host); echo ' ← fixed_http_host after check port and clean host (old logic)<br>';
var_dump($fixed_http_host_new_logic); echo ' ← fixed_http_host after check port and clean host (new logic)<br>';

$http_host .= in_array($_SERVER['SERVER_PORT'], [80, 443]) ? '' : ':' . $_SERVER['SERVER_PORT'];

$orig_http_host = in_array($_SERVER['SERVER_PORT'], [80, 443]) ? $http_host.'' : $http_host.':' . $_SERVER['SERVER_PORT'];

$fixed_http_host = !in_array($http_port, [80, 443]) ? $fixed_http_host.'' : $fixed_http_host.':' . $http_port;

$fixed_http_host_new_logic = !in_array($http_port, [80, 443]) ? $fixed_http_host_new_logic.'' : $fixed_http_host_new_logic.':' . $http_port;
echo '<br>';
var_dump($orig_http_host); echo ' ← orig_http_host after replace (old logic)<br>';
var_dump($fixed_http_host); echo ' ← fixed_http_host after replace (old logic)<br>';
var_dump($fixed_http_host_new_logic); echo ' ← fixed_http_host after replace (new logic)<br>';


////////// SPEED TESTS //////////
echo '<br>';

$time_start = microtime(true);
for($i=0;$i<=1000000;$i++){
    // Method 1 Original modx 2.8.5
		$http_host = array_key_exists('HTTP_HOST', $_SERVER) ? htmlspecialchars($_SERVER['HTTP_HOST'], ENT_QUOTES) : 'test-port.ltd:44333';
		
        if ($_SERVER['SERVER_PORT'] !== 80) {
            $http_host = str_replace(':' . $_SERVER['SERVER_PORT'], '', $http_host);
        }
        $http_host .= in_array($_SERVER['SERVER_PORT'], [80, 443]) ? '' : ':' . $_SERVER['SERVER_PORT'];
        define('MODX_HTTP_HOST', $http_host);
}
$time_end = microtime(true);
$time = $time_end - $time_start;
echo "Original modx 2.8.5 config test: ($time seconds)\n<br />";



$time_start = microtime(true);
for($i=0;$i<=1000000;$i++){
    // Method2 Fixed working with ports by dimasites
		$http_host = array_key_exists('HTTP_HOST', $_SERVER) ? parse_url($_SERVER['HTTP_HOST'], PHP_URL_HOST) : 'test-port.ltd:44333';
		
		$http_port = parse_url($_SERVER['HTTP_HOST'], PHP_URL_PORT)?:$_SERVER['SERVER_PORT'];
		
        $http_host .= in_array($http_port, [80, 443]) ? '' : ':' . $http_port;
        
		define('MODX_HTTP_HOST', $http_host);

}
$time_end = microtime(true);
$time = $time_end - $time_start;
echo "Fixed working with ports config: ($time seconds)\n";

/*
My results on PHP 7.4 about:
Hosting server 1 (client:mlk):
Original modx 2.8.5 config test: (3.4942800998688 seconds)
Fixed working with ports config: (3.3323838710785 seconds)

Hosting server 2 (main):
Original modx 2.8.5 config test: (1.6090040206909 seconds)
Fixed working with ports config: (1.4196999073029 seconds)


For URL: https://test-port.ltd/test_modx_working_with_http_ports.php
NULL ← parsed port from HTTP_HOST
string(3) "443" ← SERVER_PORT
string(3) "443" ← http_port for usage in fixed config

string(19) "test-port.ltd:44333" ← http_host (old logic)
string(13) "test-port.ltd" ← http_host (new logic)

string(15) "test-port.ltd33" ← orig_http_host after check port and clean host (old logic)
string(15) "test-port.ltd33" ← fixed_http_host after check port and clean host (old logic)
string(13) "test-port.ltd" ← fixed_http_host after check port and clean host (new logic)

string(15) "test-port.ltd33" ← orig_http_host after replace (old logic)
string(19) "test-port.ltd33:443" ← fixed_http_host after replace (old logic)
string(17) "test-port.ltd:443" ← fixed_http_host after replace (new logic)

Original modx 2.8.5 config test: (1.7936890125275 seconds)
Fixed working with ports config: (1.5702078342438 seconds)


For URL: https://test-port.ltd:44333/test_modx_working_with_http_ports.php
int(44333) ← parsed port from HTTP_HOST
string(3) "443" ← SERVER_PORT
string(3) "443" ← http_port for usage in fixed config

string(19) "test-port.ltd:44333" ← http_host (old logic)
string(13) "test-port.ltd" ← http_host (new logic)

string(15) "test-port.ltd33" ← orig_http_host after check port and clean host (old logic)
string(13) "test-port.ltd" ← fixed_http_host after check port and clean host (old logic)
string(13) "test-port.ltd" ← fixed_http_host after check port and clean host (new logic)

string(15) "test-port.ltd33" ← orig_http_host after replace (old logic)
string(13) "test-port.ltd" ← fixed_http_host after replace (old logic)
string(13) "test-port.ltd" ← fixed_http_host after replace (new logic)

Original modx 2.8.5 config test: (3.5288610458374 seconds)
Fixed working with ports config: (3.4201400279999 seconds)

*/

```
</details>

with a test and comparison of values, as well as speed tests in relation
to the replacement of string processing functions (and new logic even a
little faster)

### Related issue(s)/PR(s)
Resolves #16428 and [this hand
crutch](Pixmill-Gmbh/modx-docker@ace541e)
in [MODX Docker](https://github.com/Pixmill-Gmbh/modx-docker) project
@dimasites
Copy link
Contributor Author

Problem solved with merge of my PR, thanks all, now i close this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug The issue in the code or project, which should be addressed.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@opengeek @JoshuaLuckers @dimasites