Use the PCRE_DOLLAR_ENDONLY modifier in route regexes #25373

mpdude · Dec 7, 2017

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

UrlMatcher::match($pathinfo) applies rawurldecode() to the $pathinfo before trying to match it against the routes.

If the URL contains a percent-encoded trailing newline (like in /foo%0a), the default PHP PCRE will still consider #^/foo$# a match, as the $ metacharacter will also match immediately before the final character if it is a newline. This behavior can be changed by applying the PCRE_DOLLAR_ENDONLY modifier.

Without this change, URLs with trailing %0a lead to weird notices further down the road, for example when the RedirectableUrlMatcher or its equivalent in PhpMatcherDumper kick in, look at the last character (this time actually the newline), append a / and try to redirect to the resulting URL. Ultimately, PHP will complain with Warning: Header may not contain more than a single header, new line detected when sending the Location header.

mpdude · Dec 7, 2017

@nicolas-grekas Do you have a good hint for me why the AbstractDescriptorTest-based tests either fail on low or high PHP versions?

nicolas-grekas · Dec 8, 2017

The change impacts two components, but composer.json allows a non patched version of one of them to be used with the patched version of the other one. That's the reason.

Is there any way to make this change in a different way that doesn't needs changing the regexps? Like in the base matcher?

Tobion · Dec 10, 2017

src/Symfony/Component/Routing/Tests/RouteTest.php

-     * also works in later symfony versions, i.e. the unserialized route is in the
-     * same state as another, semantically equivalent, route.
-     */
-    public function testSerializedRepresentationKeepsWorking()


this test should be updated, not removed

I'm sorry I removed it without comment.

I don't see how this test makes sense in its current state... It checks that a Route that has been created and serialized with a former release/version of Symfony (which one?) can be unserialized and actually equals the Route, including the regexp, that the current version would generate.

This obviously fails as the regexp is changed in this PR. Fiddling with the serialized string to have it reflect the "new" regexp cannot be a solution either.

Would it suffice to check that the unserialize call itself works?

Maybe @Tobion you could explain what you intended to assert with this test?

mpdude · Dec 10, 2017

@nicolas-grekas Thanks for the hint!

I have no clever idea ATM how to achieve that... :-( Using for example trim() to get rid of the newline is not the same, as I do not want to modify the requested URL. I mean, after all, it was requested with the trailing %0a, so we should not simply ignore that.

Asking the other way round: If a change in one component breaks tests in another one, maybe that's an opportunity to re-organize/re-structure the affected tests? This somehow smells, doesn't it?

nicolas-grekas · Dec 10, 2017

On other side, changing existing tests is a sign of a BC break...

mpdude · Dec 10, 2017

Not necessarily, if the tests are too implementation specific and do not only guard intended behaviour. But let's not argue here 🍻.

Handling routes with a trailing, urlencoded newline is an edge case. I happened to discover this because someone requested such URLs on one of our apps and I found notices in the PHP log.

IMO the "right" fix would be to make PHP not silently ignore the newline at the end of the string, which I think is quirky PHP behavior in the first place.

The rest is up to you guys to decide, and as usual, you're gonna make a good decision 😇.

stof · Dec 11, 2017

src/Symfony/Component/Routing/Tests/RouteTest.php

-     * also works in later symfony versions, i.e. the unserialized route is in the
-     * same state as another, semantically equivalent, route.
-     */
-    public function testSerializedRepresentationKeepsWorking()


I'm not sure this test should be removed entirely.

@stof – yes, please see Tobias' remark and my response above.

mpdude · Dec 12, 2017

@nicolas-grekas If I get you right, the problem is that this PR needs to change the Routing component and tests/fixtures in FrameworkBundle?

The Descriptor tests in FrameworkBundle are brittle, as they fail when the route regexp changes. IMO it's not the intention of these Descriptor tests to assert the exact route regexp.

So, what if the Descriptor tests allowed placeholders/regexes in the fixtures to accept (some) changes in the output?

…s in the UrlMatcher (mpdude) This PR was squashed before being merged into the 2.7 branch (closes #25427). Discussion ---------- Preserve percent-encoding in URLs when performing redirects in the UrlMatcher | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | While investigating #25373, I found that when the *dumped* `UrlMatcher` performs redirections due to missing trailing slashes on URLs, it does so using an url*de*coded URL. This is wrong, as it may lead to wrong interpretations of URLs upon the next request. For example, think of an URL that contains `%23` in the middle of the path info. Upon redirection, this will be turned into `#` with an obvious effect. Commits ------- 8146510 Preserve percent-encoding in URLs when performing redirects in the UrlMatcher

mpdude · Jan 27, 2018

Ping @symfony/deciders

I‘d like to know whether you’re going to pull/fix this.

I am continuously getting the notices caused by this in my log monitoring. If you won’t fix it, I’ll have a look what my other options are.

I haven’t heard any objections against the fix per se. The issue is that it breaks some tests across components. The tests affected IMO never intended to assert a particular routing behavior and do so only by accident.

nicolas-grekas · Feb 3, 2018

Continued in #26035

mpdude · Feb 3, 2018

@nicolas-grekas I probably don’t fully understand the implications of what I am suggesting here.

Do we agree that the tests for FrameworkBundle are not responsible for (and never intended to) guard the regex used?

Even more, agree that the regex used is an implementation detail whereas tests should assert behavior?

Is it even impossible to change FrameworkBundle tests in a way that ignores/tolerates different regexes, as we would have to change the tests for versions that have already been released? So, the regex will never ever be changed anymore?

Could we address this by narrowing dependency version constraints on either component/bundle? Or isn’t that an option either because we cannot change them for already released versions (that is, preventing tests from “before” the change to run against a version containing the change)?

Thanks for your help, I appreciate your feedback!

nicolas-grekas · Feb 3, 2018

Yes about FrameworkBundle. Narrowing minimum version is possible, but maybe better not and make tests less strict instead.
I'm more worried by the removal of the serialized routes. We added the test for a reason, and there are serialized routes into the wild that deserve some attention.
By not changing the route data object at all, we take zero risk, thus my proposal.

mpdude · Feb 3, 2018

Ok, I don’t get this serialized routes part... you mean people have tucked away serialized routes somewhere and you would like them to continue working as-is? Why would this PR break that?

mpdude · Feb 3, 2018

I’d like to look into making FrameworkBundle tests less strict if you say that this might solve the problem.

I guess that would still require bumping deps just to make sure the tests are liberal enough?

nicolas-grekas · Feb 3, 2018

src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php

+        $collection = new RouteCollection();
+        $collection->add('foo', new Route('/foo'));
+
+        $matcher = new UrlMatcher($collection, new RequestContext());


Should use the new getUrlMatcher method

I seem to be unable to find a getUrlMatcher method somewhere...?

missing update

when this is fixed, I'll be +1 :)

nicolas-grekas · Feb 3, 2018

src/Symfony/Component/Routing/Tests/RouteTest.php

-     */
-    public function testSerializedRepresentationKeepsWorking()
-    {
-        $serialized = 'C:31:"Symfony\Component\Routing\Route":934:{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":569:{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:30:"#^/prefix(?:/(?P<foo>\d+))?$#s";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:39:"#^(?P<locale>[^\.]++)\.example\.net$#si";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";}}}}}';


Just add the missing Documents and keep the test

nicolas-grekas · Feb 3, 2018

bumping deps just to make sure the tests are liberal enough

I don't think so. Let's try without.

nicolas-grekas · Feb 4, 2018

you need to fetch+rebase on lastest 2.7

mpdude · Feb 4, 2018

@nicolas-grekas Using a RouteStub in FrameworkBundle tests, kept the Route unserialization test, rebased & squashed. Let's 🤞 and see if tests stay 🆗

Update: fabbot.io complains about fixture for generated code violating CS; ignoring that.

nicolas-grekas · Feb 4, 2018

src/Symfony/Component/Routing/Tests/RouteTest.php

@@ -272,7 +272,7 @@ public function testSerializeWhenCompiled()
     */
    public function testSerializedRepresentationKeepsWorking()
    {
-        $serialized = 'C:31:"Symfony\Component\Routing\Route":934:{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":569:{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:30:"#^/prefix(?:/(?P<foo>\d+))?$#s";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:39:"#^(?P<locale>[^\.]++)\.example\.net$#si";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":959:{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":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";}}}}}';


this should only add the "D" to the regexp, and NOT add anything else (eg not the "condition" property), because the purpose of the test is to ensure we can still unserialize old routes.

$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";}}}}}';

@nicolas-grekas Are you sure that this is correct? unserialize fails for me... Could you pastebin this somewhere?

updated above, some numbers missed an update

Works – thanks!

nicolas-grekas · Feb 4, 2018

src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php

+        $collection = new RouteCollection();
+        $collection->add('foo', new Route('/foo'));
+
+        $matcher = new UrlMatcher($collection, new RequestContext());


when this is fixed, I'll be +1 :)

nicolas-grekas · Feb 4, 2018

Thank you @mpdude.

…ude) This PR was squashed before being merged into the 2.7 branch (closes #25373). Discussion ---------- Use the PCRE_DOLLAR_ENDONLY modifier in route regexes | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | `UrlMatcher::match($pathinfo)` applies `rawurldecode()` to the `$pathinfo` before trying to match it against the routes. If the URL contains a percent-encoded trailing newline (like in `/foo%0a`), the default PHP PCRE will still consider `#^/foo$#` a match, as the `$` metacharacter will also match *immediately before* the final character *if it is a newline*. This behavior can be changed by applying the [`PCRE_DOLLAR_ENDONLY` modifier](http://php.net/manual/en/reference.pcre.pattern.modifiers.php). Without this change, URLs with trailing `%0a` lead to weird notices further down the road, for example when the `RedirectableUrlMatcher` or its equivalent in `PhpMatcherDumper` kick in, look at the last character (this time actually the newline), append a `/` and try to redirect to the resulting URL. Ultimately, PHP will complain with `Warning: Header may not contain more than a single header, new line detected` when sending the `Location` header. Commits ------- f713a3e Use the PCRE_DOLLAR_ENDONLY modifier in route regexes

mpdude · Feb 4, 2018

@nicolas-grekas Thank you for you help! Your professionalism and solution-oriented approach makes it always fun working with you.

carsonbot added Status: Needs Review Bug labels Dec 7, 2017

nicolas-grekas added this to the 2.7 milestone Dec 8, 2017

Tobion reviewed Dec 10, 2017

View reviewed changes

mpdude mentioned this pull request Dec 10, 2017

Preserve percent-encoding in URLs when performing redirects in the UrlMatcher #25427

Closed

stof reviewed Dec 11, 2017

View reviewed changes

nicolas-grekas mentioned this pull request Feb 3, 2018

[Routing] Use the PCRE_DOLLAR_ENDONLY modifier in route regexes #26035

Closed

nicolas-grekas reviewed Feb 3, 2018

View reviewed changes

Use the PCRE_DOLLAR_ENDONLY modifier in route regexes

61a8840

mpdude force-pushed the fix-url-with-trailing-encoded-newline branch from 1404434 to 61a8840 Compare February 4, 2018 11:02

nicolas-grekas reviewed Feb 4, 2018

View reviewed changes

Keep serialized route as much as-is as possible

2211f1c

nicolas-grekas requested changes Feb 4, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 4, 2018

nicolas-grekas closed this Feb 4, 2018

mpdude deleted the fix-url-with-trailing-encoded-newline branch February 4, 2018 17:15

xabbuh added the Routing label Feb 9, 2018

This was referenced Feb 28, 2018

Release v2.7.42 #26345

Closed

Release v2.7.42 #26346

Merged

Release v2.8.35 #26361

Merged

Release v3.4.5 #26362

Merged

Release v4.0.5 #26363

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

Use the PCRE_DOLLAR_ENDONLY modifier in route regexes #25373

Use the PCRE_DOLLAR_ENDONLY modifier in route regexes #25373

Uh oh!

Conversation

mpdude commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpdude commented Dec 7, 2017

Uh oh!

nicolas-grekas commented Dec 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpdude Dec 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpdude commented Dec 10, 2017

Uh oh!

nicolas-grekas commented Dec 10, 2017

Uh oh!

mpdude commented Dec 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpdude commented Dec 12, 2017

Uh oh!

mpdude commented Jan 27, 2018

Uh oh!

nicolas-grekas commented Feb 3, 2018

Uh oh!

mpdude commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Feb 3, 2018

Uh oh!

mpdude commented Feb 3, 2018

Uh oh!

mpdude commented Feb 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Feb 3, 2018

Uh oh!

nicolas-grekas commented Feb 4, 2018

Uh oh!

mpdude commented Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Feb 4, 2018

Uh oh!

mpdude commented Feb 4, 2018

Uh oh!

Uh oh!

mpdude commented Dec 7, 2017 •

edited

Loading

mpdude Dec 10, 2017 •

edited

Loading

mpdude commented Feb 3, 2018 •

edited

Loading

mpdude commented Feb 4, 2018 •

edited

Loading

nicolas-grekas Feb 4, 2018 •

edited

Loading