Skip to content

[Routing] Fix matching of utf8 params #42159

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 2 commits into from
Closed

[Routing] Fix matching of utf8 params #42159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f3da58
[Routing] Fix matching of utf8 params
mazanax Jul 19, 2021
89cc6e8
[Routing] Change route params regex pattern
mazanax Jul 19, 2021
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
33 changes: 24 additions & 9 deletions src/Symfony/Component/Routing/CompiledRoute.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
class CompiledRoute implements \Serializable
{
private $sanitizedVariables;
private $variables;
private $tokens;
private $staticPrefix;
Expand All @@ -28,16 +29,17 @@ class CompiledRoute implements \Serializable
private $hostTokens;

/**
* @param string $staticPrefix The static prefix of the compiled route
* @param string $regex The regular expression to use to match this route
* @param array $tokens An array of tokens to use to generate URL for this route
* @param array $pathVariables An array of path variables
* @param string|null $hostRegex Host regex
* @param array $hostTokens Host tokens
* @param array $hostVariables An array of host variables
* @param array $variables An array of variables (variables defined in the path and in the host patterns)
* @param string $staticPrefix The static prefix of the compiled route
* @param string $regex The regular expression to use to match this route
* @param array $tokens An array of tokens to use to generate URL for this route
* @param array $pathVariables An array of path variables
* @param string|null $hostRegex Host regex
* @param array $hostTokens Host tokens
* @param array $hostVariables An array of host variables
* @param array $variables An array of variables (variables defined in the path and in the host patterns)
* @param array $sanitizedVariables An array of sanitized variables (variables without utf8 characters)
*/
public function __construct(string $staticPrefix, string $regex, array $tokens, array $pathVariables, string $hostRegex = null, array $hostTokens = [], array $hostVariables = [], array $variables = [])
public function __construct(string $staticPrefix, string $regex, array $tokens, array $pathVariables, string $hostRegex = null, array $hostTokens = [], array $hostVariables = [], array $variables = [], array $sanitizedVariables = [])
{
$this->staticPrefix = $staticPrefix;
$this->regex = $regex;
Expand All @@ -47,12 +49,14 @@ public function __construct(string $staticPrefix, string $regex, array $tokens,
$this->hostTokens = $hostTokens;
$this->hostVariables = $hostVariables;
$this->variables = $variables;
$this->sanitizedVariables = $sanitizedVariables;
}

public function __serialize(): array
{
return [
'vars' => $this->variables,
'sanitized_vars' => $this->sanitizedVariables,
'path_prefix' => $this->staticPrefix,
'path_regex' => $this->regex,
'path_tokens' => $this->tokens,
Expand All @@ -74,6 +78,7 @@ final public function serialize(): string
public function __unserialize(array $data): void
{
$this->variables = $data['vars'];
$this->sanitizedVariables = $data['sanitized_vars'] ?? [];
$this->staticPrefix = $data['path_prefix'];
$this->regex = $data['path_regex'];
$this->tokens = $data['path_tokens'];
Expand Down Expand Up @@ -170,4 +175,14 @@ public function getHostVariables()
{
return $this->hostVariables;
}

/**
* Returns sanitized variables.
*
* @return array The sanitized variables
*/
public function getSanitizedVariables(): array
{
return $this->sanitizedVariables;
}
}
18 changes: 17 additions & 1 deletion src/Symfony/Component/Routing/Matcher/Dumper/CompiledUrlMatcherDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface;
use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
use Symfony\Component\Routing\CompiledRoute;
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;

Expand Down Expand Up @@ -434,7 +435,7 @@ private function compileRoute(Route $route, string $name, $vars, bool $hasTraili

return [
['_route' => $name] + $defaults,
$vars,
$this->restoreRouteParamNames($vars, $route->compile()),
array_flip($route->getMethods()) ?: null,
array_flip($route->getSchemes()) ?: null,
$hasTrailingSlash,
Expand Down Expand Up @@ -498,4 +499,19 @@ public static function export($value): string

return substr_replace($export, ']', -2);
}

protected static function restoreRouteParamNames($params, CompiledRoute $compiledRoute)
{
if (!\is_array($params)) {
return $params;
}

$sanitizedVariablesMap = $compiledRoute->getSanitizedVariables();
$result = [];
foreach ($params as $value) {
$result[] = $sanitizedVariablesMap[$value] ?? $value;
}

return $result;
}
}
2 changes: 2 additions & 0 deletions src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ protected function matchCollection(string $pathinfo, RouteCollection $routes)

$this->addTrace('Route matches!', self::ROUTE_MATCHES, $name, $route);

$matches = self::restoreRouteParamNames($matches, $compiledRoute);

return $this->getAttributes($route, $name, array_replace($matches, $hostMatches, $status[1] ?? []));
}

Expand Down
18 changes: 18 additions & 0 deletions src/Symfony/Component/Routing/Matcher/UrlMatcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Symfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface;
use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\CompiledRoute;
use Symfony\Component\Routing\Exception\MethodNotAllowedException;
use Symfony\Component\Routing\Exception\NoConfigurationException;
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
Expand Down Expand Up @@ -192,6 +193,8 @@ protected function matchCollection(string $pathinfo, RouteCollection $routes)
continue;
}

$matches = self::restoreRouteParamNames($matches, $compiledRoute);

return $this->getAttributes($route, $name, array_replace($matches, $hostMatches, $status[1] ?? []));
}

Expand Down Expand Up @@ -276,4 +279,19 @@ protected function createRequest(string $pathinfo): ?Request
'SCRIPT_NAME' => $this->context->getBaseUrl(),
]);
}

protected static function restoreRouteParamNames(array $params, CompiledRoute $compiledRoute): array
{
$sanitizedVariablesMap = $compiledRoute->getSanitizedVariables();
$result = [];
foreach ($params as $key => $value) {
if (\is_string($key)) {
$key = $sanitizedVariablesMap[$key] ?? $key;
}

$result[$key] = $value;
}

return $result;
}
}
34 changes: 29 additions & 5 deletions src/Symfony/Component/Routing/RouteCompiler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ public static function compile(Route $route)
$tokens = $result['tokens'];
$regex = $result['regex'];

$uniqueVariables = array_unique($variables);
$sanitizedVariables = array_combine(self::sanitizeVariables($uniqueVariables), $uniqueVariables);

return new CompiledRoute(
$staticPrefix,
$regex,
Expand All @@ -99,7 +102,8 @@ public static function compile(Route $route)
$hostRegex,
$hostTokens,
$hostVariables,
array_unique($variables)
$uniqueVariables,
$sanitizedVariables
);
}

Expand All @@ -122,10 +126,15 @@ private static function compilePattern(Route $route, string $pattern, bool $isHo

// Match all variables enclosed in "{}" and iterate over them. But we only want to match the innermost variable
// in case of nested "{}", e.g. {foo{bar}}. This in ensured because \w does not match "{" or "}" itself.
preg_match_all('#\{(!)?(\w+)\}#', $pattern, $matches, \PREG_OFFSET_CAPTURE | \PREG_SET_ORDER);
$routeParamsPattern = '#\{(!)?([\w\pL]++)\}#';
if ($needsUtf8) {
$routeParamsPattern .= 'u';
}
preg_match_all($routeParamsPattern, $pattern, $matches, \PREG_OFFSET_CAPTURE | \PREG_SET_ORDER);
foreach ($matches as $match) {
$important = $match[1][1] >= 0;
$varName = $match[2][0];
$sanitizedVarName = self::sanitizeVariable($varName);
// get all static text preceding the current variable
$precedingText = substr($pattern, $pos, $match[0][1] - $pos);
$pos = $match[0][1] + \strlen($match[0][0]);
Expand All @@ -142,7 +151,7 @@ private static function compilePattern(Route $route, string $pattern, bool $isHo

// A PCRE subpattern name must start with a non-digit. Also a PHP variable cannot start with a digit so the
// variable would not be usable as a Controller action argument.
if (preg_match('/^\d/', $varName)) {
if (preg_match('/^\d/', $sanitizedVarName)) {
throw new \DomainException(sprintf('Variable name "%s" cannot start with a digit in route pattern "%s". Please use a different name.', $varName, $pattern));
}
if (\in_array($varName, $variables)) {
Expand Down Expand Up @@ -196,9 +205,9 @@ private static function compilePattern(Route $route, string $pattern, bool $isHo
}

if ($important) {
$token = ['variable', $isSeparator ? $precedingChar : '', $regexp, $varName, false, true];
$token = ['variable', $isSeparator ? $precedingChar : '', $regexp, $sanitizedVarName, false, true];
} else {
$token = ['variable', $isSeparator ? $precedingChar : '', $regexp, $varName];
$token = ['variable', $isSeparator ? $precedingChar : '', $regexp, $sanitizedVarName];
}

$tokens[] = $token;
Expand Down Expand Up @@ -345,4 +354,19 @@ private static function transformCapturingGroupsToNonCapturings(string $regexp):

return $regexp;
}

private static function sanitizeVariables(array $variables): array
{
$result = [];
foreach ($variables as $variable) {
$result[] = self::sanitizeVariable($variable);
}

return $result;
}

private static function sanitizeVariable(string $variable): string
{
return preg_replace('#\W#', '_', $variable);
}
}
2 changes: 1 addition & 1 deletion src/Symfony/Component/Routing/Tests/Matcher/RedirectableUrlMatcherTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ public function testNonGreedyTrailingRequirement()
$this->assertEquals(['_route' => 'a', 'a' => '123'], $matcher->match('/123/'));
}

public function testTrailingRequirementWithDefault_A()
public function testTrailingRequirementWithDefaultA()
{
$coll = new RouteCollection();
$coll->add('a', new Route('/fr-fr/{a}', ['a' => 'aaa'], ['a' => '.+']));
Expand Down
18 changes: 16 additions & 2 deletions src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,7 +909,7 @@ public function testTrailingRequirementWithDefault()
$this->assertEquals(['_route' => 'b', 'b' => 'BBB'], $matcher->match('/en-en/BBB'));
}

public function testTrailingRequirementWithDefault_A()
public function testTrailingRequirementWithDefaultA()
{
$coll = new RouteCollection();
$coll->add('a', new Route('/fr-fr/{a}', ['a' => 'aaa'], ['a' => '.+']));
Expand All @@ -920,7 +920,7 @@ public function testTrailingRequirementWithDefault_A()
$matcher->match('/fr-fr/');
}

public function testTrailingRequirementWithDefault_B()
public function testTrailingRequirementWithDefaultB()
{
$coll = new RouteCollection();
$coll->add('b', new Route('/en-en/{b}', ['b' => 'bbb'], ['b' => '.*']));
Expand All @@ -941,6 +941,20 @@ public function testRestrictiveTrailingRequirementWithStaticRouteAfter()
$this->assertEquals(['_route' => 'a', '_' => '/'], $matcher->match('/hello/'));
}

public function testUtf8PatternMatchAndParameterReturn()
{
$collection = new RouteCollection();
$collection->add('foo', new Route('/foo/{bär}', [], [], ['utf8' => true]));
$matcher = $this->getUrlMatcher($collection);
try {
$matcher->match('/no-match');
$this->fail();
} catch (ResourceNotFoundException $e) {
}

$this->assertEquals(['_route' => 'foo', 'bär' => 'baz'], $matcher->match('/foo/baz'));
Copy link
Contributor

Choose a reason for hiding this comment

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

assertSame whenever possible

Copy link
Author

@mazanax mazanax Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure that it's possible to use assertSame here. $matcher->match() doesn't guarantee the order of elements in the array. And assertEquals ignores it.

}

protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null)
{
return new UrlMatcher($routes, $context ?? new RequestContext());
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Routing/Tests/RouteTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ public function testSerializeWhenCompiledWithClass()
*/
public function testSerializedRepresentationKeepsWorking()
{
$serialized = 'C:31:"Symfony\Component\Routing\Route":936:{a:8:{s:4:"path";s:13:"/prefix/{foo}";s:4:"host";s:20:"{locale}.example.net";s:8:"defaults";a:1:{s:3:"foo";s:7:"default";}s:12:"requirements";a:1:{s:3:"foo";s:3:"\d+";}s:7:"options";a:1:{s:14:"compiler_class";s:39:"Symfony\Component\Routing\RouteCompiler";}s:7:"schemes";a:0:{}s:7:"methods";a:0:{}s:8:"compiled";C:39:"Symfony\Component\Routing\CompiledRoute":571:{a:8:{s:4:"vars";a:2:{i:0;s:6:"locale";i:1;s:3:"foo";}s:11:"path_prefix";s:7:"/prefix";s:10:"path_regex";s:31:"{^/prefix(?:/(?P<foo>\d+))?$}sD";s:11:"path_tokens";a:2:{i:0;a:4:{i:0;s:8:"variable";i:1;s:1:"/";i:2;s:3:"\d+";i:3;s:3:"foo";}i:1;a:2:{i:0;s:4:"text";i:1;s:7:"/prefix";}}s:9:"path_vars";a:1:{i:0;s:3:"foo";}s:10:"host_regex";s:40:"{^(?P<locale>[^\.]++)\.example\.net$}sDi";s:11:"host_tokens";a:2:{i:0;a:2:{i:0;s:4:"text";i:1;s:12:".example.net";}i:1;a:4:{i:0;s:8:"variable";i:1;s:0:"";i:2;s:7:"[^\.]++";i:3;s:6:"locale";}}s:9:"host_vars";a:1:{i:0;s:6:"locale";}}}}}';
$serialized = 'C:31:"Symfony\Component\Routing\Route":1033:{a:9:{s:4:"path";s:13:"/prefix/{foo}";s:4:"host";s:20:"{locale}.example.net";s:8:"defaults";a:1:{s:3:"foo";s:7:"default";}s:12:"requirements";a:1:{s:3:"foo";s:3:"\d+";}s:7:"options";a:1:{s:14:"compiler_class";s:39:"Symfony\Component\Routing\RouteCompiler";}s:7:"schemes";a:0:{}s:7:"methods";a:0:{}s:9:"condition";s:0:"";s:8:"compiled";C:39:"Symfony\Component\Routing\CompiledRoute":645:{a:9:{s:4:"vars";a:2:{i:0;s:6:"locale";i:1;s:3:"foo";}s:14:"sanitized_vars";a:2:{s:6:"locale";s:6:"locale";s:3:"foo";s:3:"foo";}s:11:"path_prefix";s:7:"/prefix";s:10:"path_regex";s:31:"{^/prefix(?:/(?P<foo>\d+))?$}sD";s:11:"path_tokens";a:2:{i:0;a:4:{i:0;s:8:"variable";i:1;s:1:"/";i:2;s:3:"\d+";i:3;s:3:"foo";}i:1;a:2:{i:0;s:4:"text";i:1;s:7:"/prefix";}}s:9:"path_vars";a:1:{i:0;s:3:"foo";}s:10:"host_regex";s:40:"{^(?P<locale>[^\.]++)\.example\.net$}sDi";s:11:"host_tokens";a:2:{i:0;a:2:{i:0;s:4:"text";i:1;s:12:".example.net";}i:1;a:4:{i:0;s:8:"variable";i:1;s:0:"";i:2;s:7:"[^\.]++";i:3;s:6:"locale";}}s:9:"host_vars";a:1:{i:0;s:6:"locale";}}}}}';
$unserialized = unserialize($serialized);

$route = new Route('/prefix/{foo}', ['foo' => 'default'], ['foo' => '\d+']);
Expand Down