Commit 99bc093a authored by Oliver Hader's avatar Oliver Hader Committed by Oliver Hader
Browse files

[BUGFIX] Ensure only empty route requirements for aspects are defined

Overriding route requirements with `.+` allows to have slashes in route
parameters. This is different to Symfony's default behavior not allowing
slashes here at all.

However, when having multiple route parameters it can lead to resolving
false-positive routes like shown in the following example:

    routePath: '{first}/{second}'
    URI: https://example.com/first/second/third
    resolves to parameters
    + first: 'first/second'
    + second: 'third'

This change passes existing TYPO3 route `requirements` and uses pattern
`.+` only for those parameters not having a definition - both applies to
parameters using `aspects` only.

Besides that tests in `EnhancerLinkGeneratorTest` mixed internal argument
values with URL parameters (`100` <=> `hundred`) are were "wrong" before.

Resolves: #91246
Releases: master, 10.4, 9.5
Change-Id: Ic1fe15790cc16dd52c624cd3be9ed060ae9b9d69
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/64843

Tested-by: default avatarTYPO3com <noreply@typo3.com>
Tested-by: Oliver Hader's avatarOliver Hader <oliver.hader@typo3.org>
Reviewed-by: Oliver Hader's avatarOliver Hader <oliver.hader@typo3.org>
parent f3a4442d
......@@ -57,29 +57,32 @@ abstract class AbstractEnhancer implements EnhancerInterface
*/
protected function applyRequirements(Route $route, array $requirements, string $namespace = null)
{
if (empty($requirements)) {
return;
}
$requirements = $this->getVariableProcessor()
->deflateKeys($requirements, $namespace, $route->getArguments());
// only keep requirements that are actually part of the current route path
$requirements = $this->filterValuesByPathVariables($route, $requirements);
// In general requirements are not considered when having aspects defined for a
// given variable name and thus aspects are more specific and take precedence:
// + since requirements in classic Symfony focus on internal arguments
// Symfony's behavior on applying pattern for parameters just concerns values
// to be passed either to URL or to internal parameters - they are always the
// same, without any transformation.
//
// TYPO3 extends ("enhances") this behavior by making a difference between values
// for generation (resulting in a URL) and matching (resulting in query parameters)
// having the following implications and meaning:
//
// + since requirements in classic Symfony focus on parameters in URLs
// and aspects define a mapping between URL part (e.g. 'some-example-news')
// and the corresponding internal argument (e.g. 'tx_news_pi1[news]=123')
// + thus, the requirement definition cannot be used for resolving and generating
// a route at the same time (it would have to be e.g. `[\w_._]+` AND `\d+`)
// This call overrides default Symfony regular expression pattern `[^/]+` (see
// `RouteCompiler::compilePattern()`) with `.+` to allow URI parameters like
// `some-example-news/january` as well.
$requirements = $this->overrideValuesByAspect($route, $requirements, '.+');
if (!empty($requirements)) {
$route->setRequirements($requirements);
}
//
// Symfony's default regular expression pattern `[^/]+` (see
// `RouteCompiler::compilePattern()`) has to be overridden with `.+` to
// allow URI parameters like `some-example-news/january` as well.
//
// Existing `requirements` for TYPO3 route enhancers are not modified, only those
// that are not defined and would use Symfony's default pattern.
$requirements = $this->defineValuesByAspect($route, $requirements, '.+');
$route->setRequirements($requirements);
}
/**
......@@ -103,17 +106,37 @@ abstract class AbstractEnhancer implements EnhancerInterface
/**
* Overrides items having an aspect definition with a given
* $overrideValue in target $values array.
* $overrideValue in target $targetValue array.
*
* @param Route $route
* @param array $values
* @param string $overrideValue
* @param string $targetValue
* @return array
*/
protected function overrideValuesByAspect(Route $route, array $values, string $overrideValue): array
protected function overrideValuesByAspect(Route $route, array $values, string $targetValue): array
{
foreach (array_keys($route->getAspects()) as $variableName) {
$values[$variableName] = $overrideValue;
$values[$variableName] = $targetValue;
}
return $values;
}
/**
* Define items having an aspect definition in case they are not defined
* with a given $targetValue in target $targetValue array.
*
* @param Route $route
* @param array $values
* @param string $targetValue
* @return array
*/
protected function defineValuesByAspect(Route $route, array $values, string $targetValue): array
{
foreach (array_keys($route->getAspects()) as $variableName) {
if (isset($values[$variableName])) {
continue;
}
$values[$variableName] = $targetValue;
}
return $values;
}
......
......@@ -16,6 +16,7 @@ namespace TYPO3\CMS\Frontend\Tests\Functional\SiteHandling;
*/
use TYPO3\CMS\Core\Core\Bootstrap;
use TYPO3\CMS\Frontend\Tests\Functional\SiteHandling\Framework\Builder\ApplicableConjunction;
use TYPO3\CMS\Frontend\Tests\Functional\SiteHandling\Framework\Builder\AspectDeclaration;
use TYPO3\CMS\Frontend\Tests\Functional\SiteHandling\Framework\Builder\Builder;
use TYPO3\CMS\Frontend\Tests\Functional\SiteHandling\Framework\Builder\EnhancerDeclaration;
......@@ -897,7 +898,7 @@ class EnhancerLinkGeneratorTest extends AbstractTestCase
$this->assertGeneratedUriEquals($testSet);
}
public function routeRequirementsAreSkippedHavingAspectsDataProvider($parentSet = null): array
public function routeRequirementsHavingAspectsAreConsideredDataProvider($parentSet = null): array
{
$builder = Builder::create();
// variables (applied when invoking expectations)
......@@ -913,20 +914,42 @@ class EnhancerLinkGeneratorTest extends AbstractTestCase
->withTargetPageId(1100)
->withUrl(
VariableValue::create(
'https://acme.us/welcome/enhance/[[value]][[pathSuffix]]',
'https://acme.us/welcome/enhance/[[resolveValue]][[pathSuffix]]',
Variables::create(['pathSuffix' => ''])
)
)
)
->withApplicableSet(
VariablesContext::create(Variables::create([
'value' => 'hundred',
'resolveValue' => 100,
'value' => 100,
'resolveValue' => 'hundred',
])),
VariablesContext::create(Variables::create([
'value' => 'hundred/binary',
'resolveValue' => 1100100,
]))
'value' => 1100100,
'resolveValue' => 'hundred/binary',
])),
ApplicableConjunction::create(
VariablesContext::create(Variables::create([
'value' => 100,
'resolveValue' => 'hundred',
])),
EnhancerDeclaration::create('requirements.value=/[a-z_/]+/')->withConfiguration([
'requirements' => [
'value' => '[a-z_/]+',
]
])
),
ApplicableConjunction::create(
VariablesContext::create(Variables::create([
'value' => 1100100,
'resolveValue' => 'hundred/binary',
])),
EnhancerDeclaration::create('requirements.value=/[a-z_/]+/')->withConfiguration([
'requirements' => [
'value' => '[a-z_/]+',
]
])
)
)
->withApplicableItems($builder->declareEnhancers())
->withApplicableSet(
......@@ -940,13 +963,6 @@ class EnhancerLinkGeneratorTest extends AbstractTestCase
])
])
)
->withApplicableSet(
EnhancerDeclaration::create('requirements.value=not-match-when-having-aspect')->withConfiguration([
'requirements' => [
'value' => 'not-match-when-having-aspect',
]
])
)
->permute()
->getTargetsForDataProvider();
}
......@@ -955,9 +971,9 @@ class EnhancerLinkGeneratorTest extends AbstractTestCase
* @param TestSet $testSet
*
* @test
* @dataProvider routeRequirementsAreSkippedHavingAspectsDataProvider
* @dataProvider routeRequirementsHavingAspectsAreConsideredDataProvider
*/
public function routeRequirementsAreSkippedHavingAspects(TestSet $testSet): void
public function routeRequirementsHavingAspectsAreConsidered(TestSet $testSet): void
{
$this->assertGeneratedUriEquals($testSet);
}
......
......@@ -686,7 +686,7 @@ class EnhancerSiteRequestTest extends AbstractTestCase
$this->assertPageArgumentsEquals($testSet);
}
public function routeRequirementsAreSkippedHavingAspectsDataProvider($parentSet = null): array
public function routeRequirementsHavingAspectsAreConsideredDataProvider($parentSet = null): array
{
$builder = Builder::create();
// variables (applied when invoking expectations)
......@@ -715,7 +715,29 @@ class EnhancerSiteRequestTest extends AbstractTestCase
VariablesContext::create(Variables::create([
'value' => 'hundred/binary',
'resolveValue' => 1100100,
]))
])),
ApplicableConjunction::create(
VariablesContext::create(Variables::create([
'value' => 'hundred',
'resolveValue' => 100,
])),
EnhancerDeclaration::create('requirements.value=/[a-z_/]+/')->withConfiguration([
'requirements' => [
'value' => '[a-z_/]+',
]
])
),
ApplicableConjunction::create(
VariablesContext::create(Variables::create([
'value' => 'hundred/binary',
'resolveValue' => 1100100,
])),
EnhancerDeclaration::create('requirements.value=/[a-z_/]+/')->withConfiguration([
'requirements' => [
'value' => '[a-z_/]+',
]
])
)
)
->withApplicableItems($builder->declareEnhancers())
->withApplicableSet(
......@@ -729,13 +751,6 @@ class EnhancerSiteRequestTest extends AbstractTestCase
])
])
)
->withApplicableSet(
EnhancerDeclaration::create('requirements.value=not-match-when-having-aspect')->withConfiguration([
'requirements' => [
'value' => 'not-match-when-having-aspect',
]
])
)
->permute()
->getTargetsForDataProvider();
}
......@@ -744,9 +759,9 @@ class EnhancerSiteRequestTest extends AbstractTestCase
* @param TestSet $testSet
*
* @test
* @dataProvider routeRequirementsAreSkippedHavingAspectsDataProvider
* @dataProvider routeRequirementsHavingAspectsAreConsideredDataProvider
*/
public function routeRequirementsAreSkippedHavingAspects(TestSet $testSet): void
public function routeRequirementsHavingAspectsAreConsidered(TestSet $testSet): void
{
$this->assertPageArgumentsEquals($testSet);
}
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment