Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[Security] Fix for check_path and logout #6040

Closed
wants to merge 2 commits into from

6 participants

@omgnull

Bug fix: YES
Feature addition: NO
Backwards compatibility break: NO
Symfony2 tests pass: YES
Fixes the following tickets: #5695
License of the code: MIT

src/Symfony/Component/Security/Http/HttpUtils.php
@@ -105,7 +105,8 @@ public function checkRequestPath(Request $request, $path)
return false;
}
}
-
+ //var_dump($request->getPathInfo());

please remove debug commented line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Security/Http/HttpUtils.php
@@ -105,7 +105,8 @@ public function checkRequestPath(Request $request, $path)
return false;
}
}
-
+ //var_dump($request->getPathInfo());
+ return $path === rawurldecode($request->getPathInfo());
return $path === $request->getPathInfo();
@stloyd
stloyd added a note

This line should be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@omgnull

Done

@Tobion
Collaborator

Why this functional test? A simple unit test would be enough to check if it works.

@fabpot
Owner

I agree with @Tobion. Functional tests are very slow to run, so whenever we can write unit tests, that's far better.

@empire

I think just adding some unit tests to HttpUtilsTest is really enough.

@empire

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".
The rawurldecode is not about unicode, so the given path is not useful.

@empire

Some similar work for line #99 $parameters = $this->urlMatcher->match($request->getPathInfo()); also may be needed

@Tobion
Collaborator

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 %20.

@empire

Thanks @Tobion for your kindly comment.

@omgnull

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.

@empire

@omgnull, I think you can test your work with doing something like following asertion in HttpUtilsTests:

$this->assertTrue($utils->checkRequestPath($this->getRequest('/foo%20bar'), '/foo bar'));
$this->assertFalse($utils->checkRequestPath($this->getRequest('/foo%20bar'), '/foo%20bar'));
@empire

If my previous comment is correct, following assertion is also needed

// Plus must not decoded to space
$this->assertTrue($utils->checkRequestPath($this->getRequest('/foo+bar'), '/foo+bar')); 

// Checking unicode
$this->assertTrue($utils->checkRequestPath($this->getRequest('/%D0%B2%D1%85%D0%BE%D0%B4'), '/вход'));

In hope this comment will be helpful.

@fabpot
Owner

@omgnull Does the hints provided by @empire enough to add some unit tests? Do you need some more help?

@omgnull

@fabpot. yes. I have to find some time. This weekend I think.

@fabpot fabpot closed this in d6a402a
@fabpot fabpot referenced this pull request from a commit
@fabpot fabpot Merge branch '2.1'
* 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
3c010db
@vicb vicb referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@mmucklo mmucklo referenced this pull request from a commit
@fabpot fabpot Merge branch '2.1'
* 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
278e3ee
@hackzilla hackzilla referenced this pull request from a commit
@fabpot fabpot Merge branch '2.1'
* 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
70e366a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 17, 2012
  1. @omgnull
  2. @omgnull

    Remove debug comments

    omgnull authored
This page is out of date. Refresh to see the latest.
View
60 src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Controller/UnicodeController.php
@@ -0,0 +1,60 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\FormLoginBundle\Controller;
+
+use Symfony\Component\Security\Core\SecurityContext;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\DependencyInjection\ContainerAware;
+
+class UnicodeController extends ContainerAware
+{
+ public function loginAction()
+ {
+ // get the login error if there is one
+ if ($this->container->get('request')->attributes->has(SecurityContext::AUTHENTICATION_ERROR)) {
+ $error = $this->container->get('request')->attributes->get(SecurityContext::AUTHENTICATION_ERROR);
+ } else {
+ $error = $this->container->get('request')->getSession()->get(SecurityContext::AUTHENTICATION_ERROR);
+ }
+
+ return $this->container->get('templating')->renderResponse('FormLoginBundle:Unicode:login.html.twig', array(
+ // last username entered by the user
+ 'last_username' => $this->container->get('request')->getSession()->get(SecurityContext::LAST_USERNAME),
+ 'error' => $error,
+ ));
+ }
+
+ public function loginCheckAction()
+ {
+ throw new \RuntimeException('loginCheckAction() should never be called.');
+ }
+
+ public function logoutAction()
+ {
+ throw new \RuntimeException('logoutAction() should never be called.');
+ }
+
+ public function secureAction()
+ {
+ throw new \RuntimeException('secureAction() should never be called.');
+ }
+
+ public function profileAction()
+ {
+ return new Response('Profile');
+ }
+
+ public function homepageAction()
+ {
+ return new Response('Homepage');
+ }
+}
View
19 ...mfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/config/unicode_routing.yml
@@ -0,0 +1,19 @@
+unicode_login_path:
+ pattern: /вход
+ defaults: { _controller: FormLoginBundle:Unicode:login }
+
+unicode_check_path:
+ pattern: /аутентификация
+ defaults: { _controller: FormLoginBundle:Unicode:loginCheck }
+
+unicode_default_target_path:
+ pattern: /профайл
+ defaults: { _controller: FormLoginBundle:Unicode:profile }
+
+unicode_logout_path:
+ pattern: /выход
+ defaults: { _controller: FormLoginBundle:Unicode:logout }
+
+unicode_logout_target_path:
+ pattern: /домашняя_страница
+ defaults: { _controller: FormLoginBundle:Unicode:homepage }
View
21 ...ny/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/views/Unicode/login.html.twig
@@ -0,0 +1,21 @@
+{% extends "::base.html.twig" %}
+
+{% block body %}
+
+ {% if error %}
+ <div>{{ error.message }}</div>
+ {% endif %}
+
+ <form action="{{ path('unicode_check_path') }}" method="post">
+ <label for="username">Username:</label>
+ <input type="text" id="username" name="_username" value="{{ last_username }}" />
+
+ <label for="password">Password:</label>
+ <input type="password" id="password" name="_password" />
+
+ <input type="hidden" name="_target_path" value="" />
+
+ <input type="submit" name="login" />
+ </form>
+
+{% endblock %}
View
77 src/Symfony/Bundle/SecurityBundle/Tests/Functional/UnicodeRoutesTest.php
@@ -0,0 +1,77 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\SecurityBundle\Tests\Functional;
+
+class UnicodeRoutesTest extends WebTestCase
+{
+ /**
+ * @var string Login username
+ */
+ protected $username = 'johannes';
+
+ /**
+ * @var string Login password
+ */
+ protected $password = 'test';
+
+ /**
+ * @var array Routes helper
+ */
+ protected $routes = array(
+ 'homepage' => '/домашняя_страница',
+ 'profile' => '/профайл',
+ 'login' => '/вход',
+ 'logout' => '/выход',
+ );
+
+ public function testLoginLogoutProcedure()
+ {
+ $client = $this->createClient(array('test_case' => 'StandardFormLogin', 'root_config' => 'unicode_routes_in_firewall.yml'));
+ $client->insulate();
+
+ $crawler = $client->request('GET', $this->getRoute('login'));
+ $form = $crawler->selectButton('login')->form();
+ $form['_username'] = $this->username;
+ $form['_password'] = $this->password;
+ $client->submit($form);
+
+ $this->assertRedirect($client->getResponse(), $this->getRoute('profile'));
+ $this->assertEquals('Profile', $client->followRedirect()->text());
+
+ $client->request('GET', $this->getRoute('logout'));
+ $this->assertRedirect($client->getResponse(), $this->getRoute('homepage'));
+ $this->assertEquals('Homepage', $client->followRedirect()->text());
+ }
+
+ public function getRoute($name)
+ {
+ if (!isset($this->routes[$name])) {
+ throw new \InvalidArgumentException(sprintf('No route defined with name: %s', $name));
+ }
+
+ return $this->routes[$name];
+ }
+
+ protected function setUp()
+ {
+ parent::setUp();
+
+ $this->deleteTmpDir('StandardFormLogin');
+ }
+
+ protected function tearDown()
+ {
+ parent::tearDown();
+
+ $this->deleteTmpDir('StandardFormLogin');
+ }
+}
View
3  src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/StandardFormLogin/routing.yml
@@ -3,3 +3,6 @@ _form_login_bundle:
_form_login_localized:
resource: @FormLoginBundle/Resources/config/localized_routing.yml
+
+_form_login_unicode:
+ resource: @FormLoginBundle/Resources/config/unicode_routing.yml
View
27 src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/StandardFormLogin/unicode_routes_in_firewall.yml
@@ -0,0 +1,27 @@
+imports:
+ - { resource: ./../config/default.yml }
+
+security:
+ encoders:
+ Symfony\Component\Security\Core\User\User: plaintext
+
+ providers:
+ in_memory:
+ memory:
+ users:
+ johannes: { password: test, roles: [ROLE_USER] }
+
+ firewalls:
+ default:
+ form_login:
+ login_path: /вход
+ check_path: /аутентификация
+ default_target_path: /профайл
+ use_forward: true
+ logout:
+ path: /выход
+ target: /домашняя_страница
+ anonymous: ~
+
+ access_control:
+ - { path: '^/(?:[a-z]{2})/secure/.*', roles: ROLE_USER }
View
4 src/Symfony/Component/Security/Http/HttpUtils.php
@@ -105,8 +105,8 @@ public function checkRequestPath(Request $request, $path)
return false;
}
}
-
- return $path === $request->getPathInfo();
+
+ return $path === rawurldecode($request->getPathInfo());
}
/**
Something went wrong with that request. Please try again.