Skip to content

Commit

Permalink
[BUGFIX] Avoid Uri->__toString() swallows multi-slash paths
Browse files Browse the repository at this point in the history
Our PSR-7 Uri implementation has a bug when string
casting an Uri object: Creating a Uri from for instance
'https://example.com//' leads to 'https://example.com/'
when string casting that object. The double slash at the
end is perfectly valid and of course should not be removed.

The fun part is now that we have frontend functional
slug tests that test 'https://website.local//' error
handling and redirect behavior. Due to the old functional
test behavior that had to communicate the Uri through
a PHP process, the above Uri string cast bug has been
triggered. This leads to the situation that the test
looks as if it tested the double-slash, while in fact
it didn't. When changing v11 from php-forking based
frontend site test handling to sub request handling,
we actively implemented this behavior in testing-framework
to stay compatible.

In order to drop that hack from testing-framework,
the patch now:

* Fixes the bug in Uri
* Adds a unit test to verify (string)Uri is ok
* Adds a @todo to SlugSiteRequestTest to look at the
  actual 'https://website.local//' behavior with a
  dedicated patch.

Change-Id: Iea8d048e31b85d0d849542a7a9a55fcf5f220416
Related: #67558
Resolves: #96092
Releases: master, 11.5
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/72301
Tested-by: core-ci <typo3@b13.com>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
  • Loading branch information
lolli42 committed Nov 26, 2021
1 parent 90168ce commit c41d4e3
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 8 deletions.
5 changes: 3 additions & 2 deletions typo3/sysext/core/Classes/Http/Uri.php
Original file line number Diff line number Diff line change
Expand Up @@ -588,9 +588,10 @@ public function __toString()
}

$path = $this->getPath();
if (!empty($path)) {
$uri .= '/' . ltrim($path, '/');
if ($path !== '' && !str_starts_with($path, '/')) {
$path = '/' . $path;
}
$uri .= $path;

if ($this->query) {
$uri .= '?' . $this->query;
Expand Down
15 changes: 11 additions & 4 deletions typo3/sysext/core/Tests/Unit/Http/UriTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,21 @@ public function constructorSetsAllProperties(): void
self::assertEquals('quz', $uri->getFragment());
}

public function canSerializeToStringDataProvider(): array
{
return [
'full uri' => [ 'https://user:pass@local.example.com:3001/foo?bar=baz#quz' ],
'double slash' => [ 'https://user:pass@local.example.com:3001//' ],
];
}

/**
* @test
* @dataProvider canSerializeToStringDataProvider
*/
public function canSerializeToString(): void
public function canSerializeToString(string $uri): void
{
$url = 'https://user:pass@local.example.com:3001/foo?bar=baz#quz';
$uri = new Uri($url);
self::assertEquals($url, (string)$uri);
self::assertEquals($uri, (string)(new Uri($uri)));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ public function requestsAreRedirectedWithoutHavingDefaultSiteLanguageDataProvide
$domainPaths = [
'https://website.local/',
'https://website.local/?',
'https://website.local//',
// @todo: See how core should act here and activate this or have an own test for this scenario
// 'https://website.local//',
];

return $this->wrapInArray(
Expand Down Expand Up @@ -148,7 +149,8 @@ public function shortcutsAreRedirectedDataProvider(): array
$domainPaths = [
'https://website.local/',
'https://website.local/?',
'https://website.local//',
// @todo: See how core should act here and activate this or have an own test for this scenario
// 'https://website.local//',
];

return $this->wrapInArray(
Expand Down

0 comments on commit c41d4e3

Please sign in to comment.