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.

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

Fix URL generation #592

Merged
merged 4 commits into from
Mar 8, 2024
Merged

Fix URL generation #592

merged 4 commits into from
Mar 8, 2024

Conversation

jessarcher
Copy link
Member

@jessarcher jessarcher commented Mar 6, 2024

There are several reports of issues with the URL returned by Inertia as a result of #333.

#333 was created to support the X_FORWARDED_PREFIX header value which can be added by reverse proxies that serve a web request under a sub-path that isn't normally visible to the underlying webserver.

Unfortunately, it seems to have created a scenario where a sub-path can be doubled up. I haven't been able to figure out exactly what scenario triggers this. There are several proposed PRs to fix it, but nothing I can reproduce. In any case, I'm assuming both $request->baseUrl() and $request->getRequestUri() can contain the same sub path, which we concatenate.

Some solutions propose to use $request->fullUrl(), which constructs a full URL from the various pieces (without concatenating those specific methods). The problem is that this method includes the HTTP scheme, host, and port, and there may be users that depend on Inertia's URL not having those elements (e.g. styling active links). Other solutions combine various other methods to construct the URL, but they need to get into the weeds of adding query strings, etc.

This PR proposes to let Laravel/Symfony handle the URL construction intricacies. Unfortunately, there doesn't appear to be a method that gives us exactly what we need without the scheme and host, so I've just stripped those from the resulting URL.

Alternatives:

Closes #359
Closes #421

@@ -361,4 +361,24 @@ public function test_responsable_with_invalid_key(): void
$page['props']['resource']
);
}

public function test_the_page_url_is_prefixed_with_the_proxy_prefix(): void
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this test passed prior to the changes. It exists to ensure no regression on the issue that #333 was solving.

@jessarcher jessarcher marked this pull request as ready for review March 6, 2024 06:04
This was linked to issues Mar 6, 2024
$this->assertSame('/sub/directory/user/123', $page['url']);
}

public function test_the_page_url_doesnt_double_up(): void
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test previously failed. I took it from #360.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue could be hosting Laravel in a subdirectory, which Laravel explicitly doesn't support. That being said, the previous combination of baseUrl and getRequestUri doesn't seem correct.

@jessarcher jessarcher requested a review from reinink March 6, 2024 07:15
@rojtjo
Copy link
Contributor

rojtjo commented Mar 6, 2024

Glad to see this is being addressed!

I'm just wondering why we can't just make the URL generation configurable with a sensible default (probably the implementation of this PR)? Currently we have to either override both the ResponseFactory and Response classes, or create a custom middleware to do this.

@jessarcher
Copy link
Member Author

@rojtjo I'm happy to look at making it configurable. May I ask your use case for configuring it differently from how this PR does it?

@rojtjo
Copy link
Contributor

rojtjo commented Mar 8, 2024

The only project I ran into the double path issue was when I mounted the app on a subpath using an nginx location block, but that has since been migrated to a subdomain. So unfortunately I cannot confirm if this PR would've actually fixed the issue.

The main issue in my opinion is that there is currently no easy way to override the behavior on app level. At the time I created #503 which solved the issue for me using closures, but it would've probably been easier if we had the concept of a UrlResolver contract so we could just swap the implementation through the container.

Anyways, if this PR does actually fix the issue for everyone there is no need to go through all these hoops to retrieve the correct URL. So I guess it's best to just hold off of making it configurable for now.

@murilomagalhaes
Copy link

murilomagalhaes commented Mar 9, 2024

I can confirm that this PR fixes the issue. I'm no longer having duplicated URIs. But now i'm getting the requested URI with ASCII scaped forward slashes (%2F) inside a query string on any route.

image

image

My nginx conf:

image

@RobertBoes
Copy link
Contributor

Can confirm the behaviour here is changed. Visiting /?value=te/st in current Inertia works, no redirect happens, the History is updated correctly. But with this change visiting above URL would perform the visit normally, however the GET parameters are now escaped, resulting in the history pushstate updating the URL to /?value=te%2Fst. And I'm testing this on Valet, so no special config or subdirectories. Don't think this is desired behaviour?

@jessarcher
Copy link
Member Author

So the \Illuminate\Http\Request::fullUrl method uses the Symfony\Component\HttpFoundation::getQueryString method, which in turn passes the query string through Symfony\Component\HttpFoundation::normalizeQueryString.

The Symfony\Component\HttpFoundation::getRequestUri method we previously used returns the path and query string "raw". However, it doesn't include the X_FORWARDED_PREFIX, which is why it was prefixed with Symfony\Component\HttpFoundation::getBaseUrl, but that resulted in URL double-ups.

This feels like a game of whack-a-mole.

The only thing I can think of is to manually construct the URL, similar to how fullUrl does, but without the scheme and host and without query string encoding.

@driesvints
Copy link
Collaborator

People, just want to note that Laravel does not and will never support projects in sub directories and this should be avoided at all cost: https://laravel.com/docs/10.x/installation#directory-configuration

@RobertBoes
Copy link
Contributor

People, just want to note that Laravel does not and will never support projects in sub directories and this should be avoided at all cost: laravel.com/docs/10.x/installation#directory-configuration

That's just talking about not using a document root on a Laravel app. For example you have a example.com pointed to /home/example and then your Laravel app located at /home/example/laravel-app, which would result in a URL like example.com/laravel-app, that's unsupported yes. But setting a document-root to /home/example/laravel-app or using proxy with the x-forwarded-prefix should work without issues. So with that you could host your app at example.com/laravel-app without issues.

@driesvints
Copy link
Collaborator

@RobertBoes nope, sorry not supported.

@RobertBoes
Copy link
Contributor

@driesvints That's not what the text says tho? Along with https://github.com/laravel/framework/blob/10.x/src/Illuminate/Http/Middleware/TrustProxies.php#L22 it properly sets the prefix path.

@driesvints
Copy link
Collaborator

I'm sorry @RobertBoes but Laravel has never intended to host an app other than at the document root and root url. The routing system just isn't intended to be used with a prefix.

@RobertBoes
Copy link
Contributor

So you're saying Request::HEADER_X_FORWARDED_PREFIX does in fact not work in TrustProxies? Why is it an option in the middleware then?

@driesvints
Copy link
Collaborator

@RobertBoes look, all I'm saying is that if you expect laravel-app of example.com/laravel-app to be the root of your Laravel app that that is not something we support. Url or server wise.

@rojtjo
Copy link
Contributor

rojtjo commented Mar 12, 2024

Supported by Laravel, or not, this PR fixed the double path issue. Only issue is that it introduced a bug where the query string is being encoded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in view data / url prop Subdirectory urls broken
5 participants