Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix url matcher edge cases with trailing slash#31240

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:4.2fromarjenm:refactor-url-matcher
Apr 27, 2019
Merged

Fix url matcher edge cases with trailing slash#31240

nicolas-grekas merged 1 commit intosymfony:4.2fromarjenm:refactor-url-matcher
Apr 27, 2019

Conversation

@arjenm
Copy link
Contributor

@arjenmarjenm commentedApr 25, 2019
edited
Loading

QA
Branch?master / 4.2 (not sure whether this should've been against 4.2)
Bug fix?yes
New feature?no
BC breaks?no (I think... if so, in obscure route configurations like the ones that broke in 4.2.7 ;) )
Deprecations?no
Tests pass?yes
Fixed tickets#30721
LicenseMIT

As stated in#30721 the "redirecting" (compiled) UrlMatcher ignored host-requirements when doing a 'matched url except for trailing slash difference'-redirect.

With routes like this, you'd get 404's rather than 200's on the second routes.

host1.withTrail:path:/foo/# host2/foo would become host2/foo/ due to this partial path-matchhost:host1host2.withoutTrail:# A request for host2/foo should match this routepath:/foo# host2/foo/ does not match this pathhost:host2

This was caused by too eagerly testing whether a route could've worked with an additional trailing slash.

If it could be, that would result in an attempt to redirectbefore testing the host.

The original url would get the additional slash and prior to actually redirecting, it'd be retested against the available routes.That new url would actually match no routes, since now the host check for the first routes would actually be executed and fail the match for those routes. The adjusted path in the route does not match the second sets of routes...

This PR moves the trailing-slash check to after the host checks. I've added several tests with variants on these edge cases.

Note: This is a bug in 4.2 (and 4.3).
I fixed it against master. It appears 4.2's PhpMatcherTrait was renamed to CompiledUrlMatcherTrait in 4.3. But that made it loose its history.
So I'm not sure how to proceed from here. I can rebase it on 4.2 and do the change in PhpMatcherTrait, but that would probably fail to merge in master? I'm not nearly proficient enough in Symfony PR's to know how to solve all this ;)

@nicolas-grekas also asked for these or similar tests to be added to 3.4 as 'proof' those routes worked in that version as well. I have not (yet) done so.

The tests did work on the non-redirecting UrlMatcher without change (i.e. just running UrlMatcherTest), which implies - to me at least - the routes where properly defined and should not result in 404's simply because a RedirectableUrlMatcherInterfacecan do redirects.

Actually, since I needed to change UrlMatcher as well, I'm not convinced these bugs where totally absent in 3.4. They didn't occur with the compiled version, since the host check was the very first if-statement in that humongous if-tree it generated.

PS, the branch name is a bit dramatic ;)

@arjenmarjenm changed the titleRefactor url matcherFix url matcher edge cases with trailing slashApr 25, 2019
@nicolas-grekasnicolas-grekas added this to the4.2 milestoneApr 26, 2019
@nicolas-grekasnicolas-grekas changed the base branch frommaster to4.2April 26, 2019 07:21
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

thanks
be careful when addressing the comment, I rebased this on 4.2 so I push-forced on your fork.

@arjenm
Copy link
ContributorAuthor

Status: Needs Review

@nicolas-grekas
Copy link
Member

Thank you@arjenm.

@nicolas-grekasnicolas-grekas merged commit4fcfa9d intosymfony:4.2Apr 27, 2019
nicolas-grekas added a commit that referenced this pull requestApr 27, 2019
This PR was squashed before being merged into the 4.2 branch (closes#31240).Discussion----------Fix url matcher edge cases with trailing slash| Q             | A| ------------- | ---| Branch?       | master / 4.2 (not sure whether this should've been against 4.2)| Bug fix?      | yes| New feature?  | no| BC breaks?    | no  (I think... if so, in obscure route configurations like the ones that broke in 4.2.7 ;) )| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#30721| License       | MITAs stated in#30721 the "redirecting" (compiled) UrlMatcher ignored host-requirements when doing a 'matched url except for trailing slash difference'-redirect.With routes like this, you'd get 404's rather than 200's on the second routes.```yamlhost1.withTrail:  path: /foo/ # host2/foo would become host2/foo/ due to this partial path-match  host: host1host2.withoutTrail: # A request for host2/foo should match this route  path: /foo # host2/foo/ does not match this path  host: host2```This was caused by too eagerly testing whether a route could've worked with an additional trailing slash.If it could be, that would result in an attempt to redirect _before_ testing the host.The original url would get the additional slash and prior to actually redirecting, it'd be retested against the available routes. _That new url_ would actually match no routes, since now the host check for the first routes would actually be executed and fail the match for those routes. The adjusted path in the route does not match the second sets of routes...This PR moves the trailing-slash check to after the host checks. I've added several tests with variants on these edge cases.*Note:* This is a bug in 4.2 (and 4.3).I fixed it against master. It appears 4.2's PhpMatcherTrait was renamed to CompiledUrlMatcherTrait in 4.3. But that made it loose its history.So I'm not sure how to proceed from here. I can rebase it on 4.2 and do the change in PhpMatcherTrait, but that would probably fail to merge in master? I'm not nearly proficient enough in Symfony PR's to know how to solve all this ;)@nicolas-grekas also asked for these or similar tests to be added to 3.4 as 'proof' those routes worked in that version as well. I have not (yet) done so.The tests did work on the non-redirecting UrlMatcher without change (i.e. just running UrlMatcherTest), which implies - to me at least - the routes where properly defined and should not result in 404's simply because a RedirectableUrlMatcherInterface _can_ do redirects.Actually, since I needed to change UrlMatcher as well, I'm not convinced these bugs where totally absent in 3.4. They didn't occur with the compiled version, since the host check was the very first if-statement in that humongous if-tree it generated.PS, the branch name is a bit dramatic ;)Commits-------4fcfa9d Fix url matcher edge cases with trailing slash
@fabpotfabpot mentioned this pull requestMay 1, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

3 participants

@arjenm@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp