Skip to content
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

Regression in Security/Router Integration #2679

Closed
schmittjoh opened this issue Nov 19, 2011 · 15 comments

Comments

Projects
None yet
10 participants
@schmittjoh
Copy link
Contributor

commented Nov 19, 2011

The locale refactoring lead to a regression in the Security/Router integration. It's now again possible to detect the routes for secured parts of the website.

@fabpot

This comment has been minimized.

Copy link
Member

commented Nov 19, 2011

Could you be a bit more clear about the regression?

@schmittjoh

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2011

If you remove the highly_protected_resource route again, you will have a failing test case that shows the problem:

https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/config/routing.yml#L25

@schmittjoh

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2011

@fabpot ping, are you working on fixing this?

@vicb

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2012

@schmittjoh I think we are talking about the fact a non-existent protected URL return a 404 while they would have returned a 403 before (the same as for an existant URL).

Could you work on a fix or provide some hints on what caused the regression and how to solve it ? Thanks.

@schmittjoh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2012

It was caused by Fabien's locale refactoring. I don't know how he likes to solve this if at all, so it's better if he takes a look himself.

If it is decided to not consider this a regression, the following test case should be removed as it serves no purpose atm:
https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SecurityRoutingIntegrationTest.php#L47

@craue

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2012

I'd like to see this fixed because of #3743.

@lsmith77

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2012

this is imho a very severe issue.

i just spend half a day trying to figure out why I was getting logged out on any request after loading a specific page. it turned out the case was a missing image used by a javascript library that was triggered by the addition of a specific class on a select tag. the 404 caused the the authentication to be lost.

@lsmith77

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2013

any news on this ticket?

@lsmith77 lsmith77 referenced this issue Feb 5, 2013

Closed

Session loss #137

@canni

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2013

@lsmith77 bisecting through repo give result that this commit e3655f3 had introduced this bad behaviour; It was priority swap between firewall and router listeners... so from now one the router listener is first to came up that protected route does not exists

That commit was already a BC break there, so fixing this issue should be discussed in detail :)

one possible solution without a BC break is to listen for 404 exceptions in security bundle and replace them if they came from protected area, what you think?

@sescandell

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2013

Hi

@schmittjoh and @lsmith77 I have a question about this issue and #3743

I'm in the special situation where exception is a NotFoundHttpException. Because of Symfony logic, AFAIK, if we are in the ExceptionListener from TwigBundle, it means that request is a MASTER request (do you see any case this is not true?). So I overrided the TwigListener to change the nature of the duplicated request in this special situation:

<?php
...
class ExceptionListener implements EventSubscriberInterface
{
    ...
    public function onKernelException(GetResponseForExceptionEvent $event)
    {
        ...
        $attributes = array(
            '_controller' => $this->controller,
            'exception'   => FlattenException::create($exception),
            'logger'      => $this->logger instanceof DebugLoggerInterface ? $this->logger : null,
            'format'      => $request->getRequestFormat(),
        );

        $request = $request->duplicate(null, null, $attributes);
        $request->setMethod('GET');

        try {
            $requestType = HttpKernelInterface::SUB_REQUEST;
            if ($exception instanceof NotFoundHttpException && $event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) {
                $requestType = HttpKernelInterface::MASTER_REQUEST;
            }
            $response = $event->getKernel()->handle($request, $requestType, true);
        } catch (\Exception $e) {
            $this->logException($exception, sprintf('Exception thrown when handling an exception (%s: %s)', get_class($e), $e->getMessage()), false);

            // set handling to false otherwise it wont be able to handle further more
            $handling = false;

            // re-throw the exception from within HttpKernel as this is a catch-all
            return;
        }

        $event->setResponse($response);

        $handling = false;
    }

    public static function getSubscribedEvents()
    {
        return array(
            KernelEvents::EXCEPTION => array('onKernelException', -127),
        );
    }
...
}

But I'm not sure what it could have as consequeses about creating a MASTER REQUEST in a MASTER REQUEST.

Doing like this seems to solve my issue (having context initialized in my ExceptionController). But I don't know what it can imply.

Do you have any idea or some tips about this "patch" in an application?
Could you please give me some feedbacks about this "hack"?

Thank you,

@wouterj

This comment has been minimized.

Copy link
Member

commented Jul 25, 2014

I have a hard time understanding this issue. If I'm correct:

  • Assume ^/admin is secured
  • Someone has access to ^/admin/post/new$, but not to ^/admin/settings$
  • /admin/settings/specific does not exists

This results in:

User visits ... Http Error Code
Symfony 2.0
/admin/settings/general 403
/admin/settings/specific 403
Symfony >2.1
/admin/settings/general 403
/admin/settings/specific 404

This means the user can find protected URLs and that seems to be a problem. Besides that, @lsmith77 mentioned that a user is logged out when he gets a 404 error code?

If so, what has the mentioned test to do with this problem? That tests the case that isn't changed at all: accessing an existing file which the user is not allowed to access.

If I'm correct until this moment, I think the solution would be a very straight forward solution: Register a listener to kernel.exception and let it execute the firewall if the exception is a 404 error. This way, it'll show a 403 error page when the user did not have access and otherwise, it'll just show the 404 error.

Can someone confirm this?

@sstok

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2014

The security is because according to the access-rules its not allowed, which should result in a 403.
But because the Security firewall listener is run after the routing process (which in the past was done later) you get a 404 as the Security listener is not run due to the 404 http exception.

And because the Firewall listener is not run, there is no security context.

But if you change this back to the old situation, then the locale is not available for the 403 page 😆

Its only a problem because a 404 can happen trough an absolutely none-existent path, which is what it is, or by getting a none existent resource which is a potential security problem.

So you're suggestion for running a Security listener on an kernel.exception is indeed our best shot.

@wouterj

This comment has been minimized.

Copy link
Member

commented Jul 25, 2014

Ok, thanks for clarifying it to me @sstok. I'll create a PR for it :)

@gquemener

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2014

Does defining access rule against route pattern that doesn't match any route in the application is a real use case?

Otherwise, it'd probably be easier to prevent defining such rules than trying to convert 404 into 403...

EDIT: answer

@fabpot

This comment has been minimized.

Copy link
Member

commented Dec 29, 2014

Closing as there is no way to "fix" this as I said numerous times and because @wouterj tried some time ago without success.

@fabpot fabpot closed this Dec 29, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.