-
Notifications
You must be signed in to change notification settings - Fork 264
Page URL: Fix handling of base url and reverse proxy forward prefix #566
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.
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
Conversation
Related PRsI saw there are a couple of open pull requests here addressing the same issue:
User impactAdditionally to the examples above, the "inertia duplicates the base url" problem also came up in different forms on stackoverflow, e.g. Ziggy router modifying browser urls or root directory is displayed twice in URL on load and reload Many thanks to the entire Inertia team for this amazing project! I hope this PR can contribute to solving one of the very few longstanding issues. |
| $page = [ | ||
| 'component' => $this->component, | ||
| 'props' => $props, | ||
| 'url' => $request->getBaseUrl().$request->getRequestUri(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be changed to just using fullUrl(), https://github.com/laravel/framework/blob/10.x/src/Illuminate/Http/Request.php#L116-L123
| 'url' => $request->getBaseUrl().$request->getRequestUri(), | |
| 'url' => $request->fullUrl(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fullUrl correctly takes care of the base url and forward prefix, but it also prepends the protocol and hostname. This may have an impact on the inertia clients for the different JavaScript frameworks. It causes 10 failures in the ResponseTest, for example:
There were 10 failures:
1) Inertia\Tests\ResponseTest::test_server_response
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'/user/123'
+'http://localhost/user/123'
/ws/dev/inertia-laravel/tests/ResponseTest.php:46
/ws/dev/inertia-laravel/vendor/orchestra/testbench-core/src/PHPUnit/TestCase.php:42
....
The advantage of using fullUrl is that it would be more consistent, since the prop is already called url but in reality only contains the path URI. But it is a potentially breaking change if JS clients assume they receive only the path URI. Also, detecting the protocol (https?) and actual client facing hostname can be a headache in a reverse proxy environment...
@RobertBoes what do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah, you're right, didn't think about that
Also, detecting the protocol (https?) and actual client facing hostname can be a headache in a reverse proxy environment...
It shouldn't be, if the correct headers are sent and the TrustProxies middleware is set up properly (which is what you should do anyways if you're using a reverse proxy)
|
At this point i've just given up for any PR that fixes this problem to be merged. Sadly, i've seen a lot of them being closed or just ignored. |
|
Following the discussion with @RobertBoes, I changed the code to make explicit that - in line with the inertia protocol example - the The code now reads: 'url' => str_replace($request->getSchemeAndHttpHost(), '', $request->getUri()),I would have liked use the |
|
Thanks for the PR, and I'm sorry for the delay! I ended up going with a different but very similar approach (#592), but this PR was very helpful! |
The
urlproperty of of the inertia HTTP response should contain the user-facing request URI, i.e. the absolute URL path and if applicable the query string.Inertia currently has a bug where, if the laravel application has base url different from root, the
urlproperty contains the baseUrl twice (#372, #359, #421). This was introduced as a side-effect of a fix for a separate issue when the laravel application is deployed behind a reverse proxy, and where the reverse proxy has a forward prefix (#333).This PR fixes the handling of the baseUrl case and preserves the behavior in case of reverse proxy with forward prefix. It also adds test cases for both cases.
Example: Base URL duplicated by Inertia response
There is a thread on laracasts, URL duplicates in shared hosting deployed Inertia App, which illustrates the base url problem:
This problem happens because the
$request->getRequestUri()contains the full request URI as sent to the application server, which also includes the base url:inertia-laravel/src/Response.php
Line 102 in 12e7937
There are more users who experienced this problem, e.g. root directory is displayed twice in URL on load and reload
Example: Reverse Proxy Deployment with Forward Prefix
Suppose the application has a public URL
https://intranet.example.org/to/project. This is served through a reverse proxy, which requests the content in the background fromhttp://localhost:8080.Using the
X-Forwarded-Prefixheader, the reverse proxy can instruct symfony to prepend the forward prefix to the baseUrl. This was the setting reported in #333.Since the forward prefix is not included in the
getRequestUri()the problem of a duplicated base url does not arise in this case.