Skip to content

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.

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

Closed
wants to merge 5 commits into from
Closed

Page URL: Fix handling of base url and reverse proxy forward prefix #566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
458cdb8
Add tests for page.url with base url and reverse proxy trusted prefix
flexponsive Dec 2, 2023
ab11f87
Fix: page.url contains the base url twice
flexponsive Dec 2, 2023
9f201d0
add test case for page url with proxy forwarded prefix only
flexponsive Dec 2, 2023
262ec7c
skip proxy forward prefix on legacy symfony which does not support th…
flexponsive Dec 2, 2023
1948dc3
make explicit that we are removing scheme and host from the full requ…
flexponsive Dec 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Response.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public function toResponse($request)
$page = [
'component' => $this->component,
'props' => $props,
'url' => $request->getBaseUrl().$request->getRequestUri(),
Copy link
Contributor

@RobertBoes RobertBoes Dec 5, 2023

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

Suggested change
'url' => $request->getBaseUrl().$request->getRequestUri(),
'url' => $request->fullUrl(),

Copy link
Author

@flexponsive flexponsive Dec 5, 2023 •
edited
Loading

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?

Copy link
Contributor

@RobertBoes RobertBoes Dec 6, 2023

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)

'url' => str_replace($request->getSchemeAndHttpHost(), '', $request->getUri()),
'version' => $this->version,
];

Expand Down
54 changes: 54 additions & 0 deletions tests/ResponseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,4 +361,58 @@ public function test_responsable_with_invalid_key(): void
$page['props']['resource']
);
}

public function test_page_url_with_baseurl() : void
{
$request = Request::create('/app-base-url/user/123', 'GET', [], [], [], [
'SCRIPT_FILENAME' => '/ws/test/app-base-url/public/index.php',
'SCRIPT_NAME' => '/app-base-url/index.php',
]);
$request->headers->add(['X-Inertia' => 'true']);
$this->assertSame('/app-base-url', $request->getBaseUrl());

$user = (object) ['name' => 'Jonathan'];
$response = new Response('User/Edit', ['user' => $user], 'app', '123');
$response = $response->toResponse($request);
$page = $response->getData();

$this->assertSame(
'/app-base-url/user/123',
$page->url
);
}

public function test_page_url_with_proxy_forwarded_prefix() : void
{
try {
Request::setTrustedProxies([
'8.8.8.8'
], Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_PREFIX);
} catch (\Throwable $th) {
$this->markTestSkipped('Trusted proxies not supported in this version of Symfony');
return;
}

$request = Request::create('/user/123', 'GET', [], [], [], [
'SCRIPT_FILENAME' => '/ws/test/app-base-url/public/index.php',
'SCRIPT_NAME' => '/index.php',
'REMOTE_ADDR' => '8.8.8.8',
'HTTP_X_FORWARDED_FOR' => '7.7.7.7',
'HTTP_X_FORWARDED_PREFIX' => '/proxy-prefix',
]);

$request->headers->add(['X-Inertia' => 'true']);

$this->assertTrue($request->isFromTrustedProxy());

$user = (object) ['name' => 'Jonathan'];
$response = new Response('User/Edit', ['user' => $user], 'app', '123');
$response = $response->toResponse($request);
$page = $response->getData();

$this->assertSame(
'/proxy-prefix/user/123',
$page->url
);
}
}