Skip to content

Commit

Permalink
merged branch Seldaek/router_esc (PR #1471)
Browse files Browse the repository at this point in the history
Commits
-------

418d6a0 [Routing] Fix syntax error when dumping routes with single quotes in the requirements or pattern
2b5e22d [Routing] Fix ApacheDumper when a space appears in a default value
6c7f484 [Routing] Fix dumper so it doesn't print trailing whitespace
761724a [Routing] Adjust urlescaping rules, fixes #752

Discussion
----------

[Router] Bunch o' Fixes

The first commit changes the escaping rule to fix issues I had previously, and #752 as well, here's from the full commit message:

    Only + and % are now encoded in generated routes, since they are the only characters that, if not encoded, could cause problems/conflicts when decoded. Namely + turns into a space, and % followed by numbers could do funky things.

    The matcher decodes everything which works since nothing will have %NN without being escaped, and + are escaped as well.

Second commit is just a test fix for the first

Third and fourth are simply dumper escaping issues, nothing to argue about.

Note that all changes have had test cases added, and I spent a few hours torturing/testing all this stuff with both Apache and PHP dumpers, in many browsers, and with URLs as wonky as `/%01%02%03%04%05%06%07%08%0E%0F%10%11%12%13%14%15%16%17%18%19%1A%1B%1C%1D%1E%1F%20!%22$%25&%27%28%29*+,-./0123456789:;%3C=%3E@ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B|%7D~/baz` which essentially represent the 1-255 char range minus ? and #.

The only issues I really encountered after all the patches were applied is that Apache refuses to match `%22` (= `"`) and `*` in a url. I guess it's just because they're not allowed chars in windows paths, but | and < > works fine though. Anyway this works with the PHP dumper, and it didn't work either without my patches so it's not like I broke it, I'm just saying for the record.
  • Loading branch information
fabpot committed Jul 1, 2011
2 parents 09007ba + 06ec884 commit d005768
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 8 deletions.
3 changes: 1 addition & 2 deletions Generator/UrlGenerator.php
Expand Up @@ -125,8 +125,7 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
}

if (!$isEmpty || !$optional) {
// %2F is not valid in a URL, so we don't encode it (which is fine as the requirements explicitly allowed it)
$url = $token[1].str_replace('%2F', '/', rawurlencode($tparams[$token[3]])).$url;
$url = $token[1].strtr($tparams[$token[3]], array('%'=>'%25', '+'=>'%2B')).$url;
}

$optional = false;
Expand Down
1 change: 1 addition & 0 deletions Matcher/Dumper/ApacheMatcherDumper.php
Expand Up @@ -62,6 +62,7 @@ public function dump(array $options = array())
':' => '\\:',
'=' => '\\=',
'\\' => '\\\\',
' ' => '\\ ',
));
}
$variables = implode(',', $variables);
Expand Down
17 changes: 11 additions & 6 deletions Matcher/Dumper/PhpMatcherDumper.php
Expand Up @@ -61,6 +61,7 @@ private function addMatcher($supportsRedirections)
public function match(\$pathinfo)
{
\$allow = array();
\$pathinfo = urldecode(\$pathinfo);
$code
throw 0 < count(\$allow) ? new MethodNotAllowedException(array_unique(\$allow)) : new ResourceNotFoundException();
Expand Down Expand Up @@ -113,14 +114,18 @@ private function compileRoutes(RouteCollection $routes, $supportsRedirections, $
}

if ($prefix !== $parentPrefix) {
$code[] = sprintf(" if (0 === strpos(\$pathinfo, '%s')) {", $prefix);
$code[] = sprintf(" if (0 === strpos(\$pathinfo, %s)) {", var_export($prefix, true));
$indent = ' ';
}
}

foreach ($this->compileRoutes($route, $supportsRedirections, $prefix) as $line) {
foreach (explode("\n", $line) as $l) {
$code[] = $indent.$l;
if ($l) {
$code[] = $indent.$l;
} else {
$code[] = $l;
}
}
}

Expand All @@ -146,22 +151,22 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
$matches = false;
if (!count($compiledRoute->getVariables()) && false !== preg_match('#^(.)\^(?P<url>.*?)\$\1#', str_replace(array("\n", ' '), '', $compiledRoute->getRegex()), $m)) {
if ($supportsRedirections && substr($m['url'], -1) === '/') {
$conditions[] = sprintf("rtrim(\$pathinfo, '/') === '%s'", rtrim(str_replace('\\', '', $m['url']), '/'));
$conditions[] = sprintf("rtrim(\$pathinfo, '/') === %s", var_export(rtrim(str_replace('\\', '', $m['url']), '/'), true));
$hasTrailingSlash = true;
} else {
$conditions[] = sprintf("\$pathinfo === '%s'", str_replace('\\', '', $m['url']));
$conditions[] = sprintf("\$pathinfo === %s", var_export(str_replace('\\', '', $m['url']), true));
}
} else {
if ($compiledRoute->getStaticPrefix() && $compiledRoute->getStaticPrefix() != $parentPrefix) {
$conditions[] = sprintf("0 === strpos(\$pathinfo, '%s')", $compiledRoute->getStaticPrefix());
$conditions[] = sprintf("0 === strpos(\$pathinfo, %s)", var_export($compiledRoute->getStaticPrefix(), true));
}

$regex = str_replace(array("\n", ' '), '', $compiledRoute->getRegex());
if ($supportsRedirections && $pos = strpos($regex, '/$')) {
$regex = substr($regex, 0, $pos).'/?$'.substr($regex, $pos + 2);
$hasTrailingSlash = true;
}
$conditions[] = sprintf("preg_match('%s', \$pathinfo, \$matches)", $regex);
$conditions[] = sprintf("preg_match(%s, \$pathinfo, \$matches)", var_export($regex, true));

$matches = true;
}
Expand Down
2 changes: 2 additions & 0 deletions Matcher/UrlMatcher.php
Expand Up @@ -93,6 +93,8 @@ public function match($pathinfo)

protected function matchCollection($pathinfo, RouteCollection $routes)
{
$pathinfo = urldecode($pathinfo);

foreach ($routes as $name => $route) {
if ($route instanceof RouteCollection) {
if (false === strpos($route->getPrefix(), '{') && $route->getPrefix() !== substr($pathinfo, 0, strlen($route->getPrefix()))) {
Expand Down

0 comments on commit d005768

Please sign in to comment.