-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] Fix for check_path and logout #6040
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ls on non ASCII symbols
@@ -105,7 +105,8 @@ public function checkRequestPath(Request $request, $path) | |||
return false; | |||
} | |||
} | |||
|
|||
//var_dump($request->getPathInfo()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove debug commented line
Done |
Why this functional test? A simple unit test would be enough to check if it works. |
I agree with @Tobion. Functional tests are very slow to run, so whenever we can write unit tests, that's far better. |
I think just adding some unit tests to HttpUtilsTest is really enough. |
The tests doesn't check functionality of rawurldecode function, the test must check something like "foo%20bar%40baz" for route with path like "foo bar@baz". |
Some similar work for line #99 $parameters = $this->urlMatcher->match($request->getPathInfo()); also may be needed |
@empire no, the matcher url decodes itself. The rawurldecode is partly about unicode, because multibyte chars get encoded as octets, too. so the test is useful. but the focus here should indeed more be about those standard encodings like |
Thanks @Tobion for your kindly comment. |
I don't have a clue what should I do in the unit test, I need any advice for this. In current functional test web client all request and redirects urls like "/%D0%B2%D1%85%D0%BE%D0%B4". May be just add some kind symbol collecton to test. |
@omgnull, I think you can test your work with doing something like following asertion in HttpUtilsTests:
|
If my previous comment is correct, following assertion is also needed
In hope this comment will be helpful. |
@fabpot. yes. I have to find some time. This weekend I think. |
* 2.1: fixed CS fixed CS [Security] fixed path info encoding (closes #6040, closes #5695) [HttpFoundation] added some tests for the previous merge and removed dead code (closes #6037) Improved Cache-Control header when no-cache is sent removed unneeded comment Fix to allow null values in labels array fix date in changelog removed the Travis icon (as this is not stable enough -- many false positive, closes #6186) Revert "merged branch gajdaw/finder_splfileinfo_fpassthu (PR #4751)" (closes #6224) Fixed a typo Fixed: HeaderBag::parseCacheControl() not parsing quoted zero correctly [Form] Fix const inside an anonymous function [Config] Loader::import must return imported data [DoctrineBridge] Fixed caching in DoctrineType when "choices" or "preferred_choices" is passed [Form] Fixed the default value of "format" in DateType to DateType::DEFAULT_FORMAT if "widget" is not "single_text" [HttpFoundation] fixed a small regression Conflicts: src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php
Bug fix: YES
Feature addition: NO
Backwards compatibility break: NO
Symfony2 tests pass: YES
Fixes the following tickets: #5695
License of the code: MIT