getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.32k stars 168 forks source link

Panel does not work when using non standard http port #3645

Closed thathoff closed 3 years ago

thathoff commented 3 years ago

Describe the bug

When accessing the panel from a non standard HTTP port (443 or 80) the panel fails to load with the following console message:

Uncaught (in promise) DOMException: Failed to execute 'pushState' on 'History': A history state object with URL 'http://localhost/panel/site' cannot be created in a document with origin 'http://localhost:8090' and URL 'http://localhost:8090/panel/login

Kirby Version
3.6.0-alpha.4

lukasbestle commented 3 years ago

Our Fiber implementation in the Panel frontend seems to use 1:1 the URL that is provided by the backend. So the question is why the backend returns a URL without the correct port. Normally it is supposed to auto-generate the URLs based on the request data. And if the request is received on that same port, Kirby should auto-detect that.

@thathoff Do you use any custom urls or roots setup in your site's index.php? What is your server setup: PHP's built-in web server or another web server (if so, which one and does it serve PHP directly or does it act as a reverse proxy)?

thathoff commented 3 years ago

Thank you! I’ll have a look at this and come back with some more information.

The setup this occurred was nginx+phpfpm inside docker. And the port has been changed by binding the containers port and not by changing the nginx’s port. Maybee that’s the problem. And the url is set manually, indeed. But it worked flawlessly with Kirby 3.5. I’ll dig deeper and keep you updated.

distantnative commented 3 years ago

@thathoff any insights?

thathoff commented 3 years ago

Hi, sorry for not coming back to this. Have been sick the last days and had no time do dig deeper into this. I’ll try it with the latest beta und check if we need to slightly modify our setup or if this is a bug in Kirby.

thathoff commented 3 years ago

So now I have new information. We run Kirby inside a docker container which binds the internal port 80 to 8090 on the dev machine. PHP and nginx still think they’re reachable on port 80 so $_SERVER['SERVER_PORT'] is still 80.

We have the real port available as an ENV var so adding the following to the config.php fixes the problem.

if (!empty($_SERVER['DEV_HTTP_PORT'])) {
    $_SERVER['SERVER_PORT'] = $_SERVER['DEV_HTTP_PORT'];
}

But this applies only to our special dev setup. A more safer way to detect the real port might be to parse $_SERVER['HTTP_HOST'] which contains the full hostname including the port if set.

thathoff commented 3 years ago

I just dug a bit deeper: The problem seems to be related to the Url::current() method. This does not include the port (even with the fix applied above). Because of this json.$url also has no port which then causes the History.pushState error.

Fun fact: Url::index() does include the port (after setting $_SERVER['SERVER_PORT']).

thathoff commented 3 years ago

More Updates: The reason Url::current() does not include the changed port is that it is initialised before the config is loaded. When I move the overriding of $_SERVER['SERVER_PORT'] to the index.php before require 'kirby/bootstrap.php'; it includes the port.

I think using $_SERVER['HTTP_HOST'] to detect the actual HTTP port might be a more fail-safe option eg. in cases where Kirby runs behind a reverse proxy on a different port.

Here’s a fix without using any special ENV variables. This needs to be put in before require 'kirby/bootstrap.php'; inside the index.php

$colonPos = !empty($_SERVER['HTTP_HOST']) ? strpos($_SERVER['HTTP_HOST'], ":") : false;
if ($colonPos !== false) {
    $_SERVER['SERVER_PORT'] = substr($_SERVER['HTTP_HOST'], $colonPos + 1);
}
philipmarnef commented 3 years ago

I'm also running Kirby in an nginx-php-fpm docker container combo mapped to port 4000. After adding 'url' => 'http://localhost:4000' in config, the site works, except for the panel which fails with ' DOMException: The operation is insecure.' The above code by @thathoff solves that.

If I use a 'localdomain.test:4000' host entry and 'url' param in config however, the panel fails with a bunch of CORS errors because it tries to load all assets @ localhost.

distantnative commented 3 years ago

A lot of comments on how to solve this on the PHP side via config...

However, I actually think this bug is rooted in the new Fiber implementation on the JS side, given the initial 'pushState' on 'History' error message. I would assume it originates on these lines: https://github.com/getkirby/kirby/blob/develop/panel/src/fiber/index.js#L220-L224

We are passing the optional third url parameter here. One question is why this.state.$url is http://localhost/panel/site for @thathoff while it should be http://localhost:8090/panel/log. So maybe we are back at the PHP side of things if this is a more general problem of not including the port.

However, if this is only about Fiber - we might be able to get around this. The docs for the url parameter

The new history entry's URL is given by this parameter. Note that the browser won't attempt to load this URL after a call to pushState(), but it might attempt to load the URL later, for instance after the user restarts the browser. The new URL does not need to be absolute; if it's relative, it's resolved relative to the current URL. The new URL must be of the same origin as the current URL; otherwise, pushState() will throw an exception. If this parameter isn't specified, it's set to the document's current URL.

Maybe passing a relative URL instead of the absolute URL circumvents our missing port problems already?

distantnative commented 3 years ago

Ok digging deeper, I also understand your, @thathoff, replies better:

I am surprised this hasn't caused any problems before 3.6 - this seems to be an issue around for longer. But maybe it really only matters with Fiber's new History.pushState call. Which then we might just circumvent there for now as a quick patch?

thathoff commented 3 years ago

Just wanted to answer basically the same you just wrote! 😂 And yes: It also caused “problems” before. We always needed to set url to Url::scheme() . '://' . $_SERVER['HTTP_HOST'] in config.php to teach Kirby about the port otherwise all asset and media urls did not include the port.

PS: Just verified that we also still need to set url manually in 3.6-rc.2 to make media and assets work properly.

distantnative commented 3 years ago

Ok so it seems to be a deep-rooted issue that the port isn't recognised automatically but pre-3.6 setting it in the url option solved all immediate issues, while with 3.6 we still see the History.pushState issue.

Next:

thathoff commented 3 years ago

Yes, thats how I would describe the problem. And from my point of view the reason is that the Webserver still runs on port 80 but the port is changed by the Docker NAT. So $_SERVER['SERVER_PORT'] still remains 80. $_SERVER['HTTP_HOST'] on the other hand is supplied by the client HTTP header which “sees” the different port and sends the HTTP header accordingly. I just checked: There is no other key inside the $_SERVER array except HTTP_HOST containing the proper client facing HTTP port.

distantnative commented 3 years ago

Maybe we can solve it directly in https://github.com/getkirby/kirby/blob/main/src/Http/Server.php#L120-L129

@thathoff would be great if you could help testing this... maybe something like

public static function port(bool $forwarded = false): int
{
    // based on forwarded port
    if ($forwarded === true) {
        if ($port = static::get('HTTP_X_FORWARDED_PORT')) {
            return $port;
        }
    }

    // based on HTTP host
    $host = static::get('HTTP_HOST');
    if ($pos = strpos($host, ':')) {
        return substr($host, $pos + 1);
    }

    // based on server port
    return static::get('SERVER_PORT');
}
thathoff commented 3 years ago

Yes, this fixes “everything”. Both the panel problem introduced in 3.6 and the need to overwrite url. 👍

distantnative commented 3 years ago

I'll get a PR ready to also add unit tests.

thathoff commented 3 years ago

Great, I'll also check rc.3 with our setup if it gets released!

bastianallgeier commented 3 years ago

tobimori commented 3 years ago

Getting a similar error with rc.3. I proxy the dev server with browser-sync for automatic reloading and have a function that sets a custom URL if specific headers are present - worked in 3.5 so far.

distantnative commented 3 years ago

@tobimori can you post your exact setup etc - otherwise hard to follow

tobimori commented 3 years ago

@tobimori can you post your exact setup etc - otherwise hard to follow

Hej, sorry for the late reply. You can reproduce it with the following custom Plainkit: https://github.com/coralic/kirby-plainkit

Install composer dependencies, then yarn dependencies, and start the dev server with yarn dev.

Edit: This seems like an issue with PHPs built-in server. It works fine when proxying Laravel Valet.