Do not merge this yet, I've found a big problem with this PR :)

I know this is a difficult issue to solve, but from a user-perspective, I hope this will work out :).

I don't get the purpose of this PR.

Currently, when an access control is set on a path that is not defined in the routing it will return a 401 response when trying to access it without the correct permission, instead of a 404?

@gquemener yes. In a protected area, you should always return a 401.
Otherwise you can find out existing URLs by just brute forcing HTTP requests against some URLs (you can check whether you will receive a 404 oder 401).

The URL might contain (in some way) sensitive data, so this is a privacy issue.

If I understand the case correctly, you're effectively trying to change the exception into an access denied based on the URL and permissions.

Checking kernel.exception is already too late. What you want to do is change the exception being thrown by the routing if access is denied.

I'm quite confused here.
The access checking is performed on the kernel request event which occurs before the controller corresponding to the route is resolved (which throw the 404 when it cannot be resolved).

In that case, I don't see how a 404 could be returned when the firewall detects an unauthorized access.

In that case, I don't see how a 404 could be returned when the firewall detects an unauthorized access.

It's not about the 404 errors thrown by a controller. When a router can't find a route, it'll throw a 404 causing exception too. Since the firewall is registered after the router (so we can use routes for the login path and such), this exception will be thrown first and the firewall never gets executed. That's why people see the 404 exception. We have to find a way to transform those 404 exceptions into 403 exceptions.

Checking kernel.exception is already too late. What you want to do is change the exception being thrown by the routing if access is denied.

We can't change the routing exception, since the Routing shouldn't do anything with security. And we can't place it before the router, since we loose the ability to use routes then.

Ah allright, thanks @wouterj, I forgot about this RouterListener.

Crazy idea: Forbid to set access_control on route that does not exist.

Crazy idea: Forbid to set access_control on route that does not exist.

That's not going to solve the issue. Assume an access_control like: ^/admin

Assume the admin can edit blog articles. That URL would be something like /admin/blog/post/{id}/edit. Now, assume a post with id 200 does not exists (since it only has 50 blog posts), you get a 404 error when visiting /admin/blog/post/200/edit. You should get a 401 error. This way, you can find out exactly how many posts are not yet published.
That isn't that problematic, but there might be more private URLs too.

In the example you give,
if you have set an access_control on ^/admin and you try to access /admin/blog/post/200/edit without the correct access attributes, you'll get a 401 (because the route/admin/blog/post/{id}/edit has been defined in the routing).

The only case that is not handled correctly today is when setting an access_control with a pattern that doesn't match any route in the application. I'm wondering if it makes sense to set access control with such pattern.

Doesn't it mean that one protects access of... well... nothing?

Then I don't understand what @schmittjoh tried to say in #2679 ...

I think you clarified very well the issue in #2679 (comment)

I'm just concerned about the real use case of defining access rule on pattern that will never be matched against any route in the application.

If there's no such use case, then it'll probably be easier to prevent user to configure access rules on invalid route pattern, than trying to convert 404 into 403.

@gquemener I got it now. If you define an access rule for ^/admin and you have a controller for /admin/settings/general, but not for /admin/settings/specific, you get a 404 error when visiting /admin/settings/specific, while a 401 was expected.

Aaah that's right, that's why my solution is not acceptable! Good catch @wouterj!

So, it seems that your solution is quite acceptable then. Let me know if you need some help with "the BIG problem with this PR" 😉 (see #11480 (comment))

So, it seems that your solution is quite acceptable then. Let me know if you need some help with "the BIG problem with this PR" 😉

The problem:

• The AccessDeniedException is transformed to a 401 result code by the ExceptionListener of the Security component
• This ExceptionListener is only registered when the firewall is executed (in onKernelRequest)
• When the router cannot find the route, the firewall will not be executed
• We then get into the exception event and onKernelException will be executed
• This executes the firewall, which wants to add the ExceptionListener to the kernel.exception event
• Since we are already in that event, the EventDispatcher fails.

So there is no way we can handle the AccessDeniedException in kernel.exception...

I was having a BrainstormSessionInterface with myself so here we go.

At the moment, we rely on an Exception to be thrown when a 404 is triggered. Because an exception is thrown, the kernel.exception is triggered. This prevents the framework to continue it's normal event flow and goes into error mode. We then have listeners to handle what's going on, a 404 might show a page or redirect. A 401 will forward to login. A 403 will show an access denied page. We have a custom listener that initializes symfony1 as fallback.

What if, instead of throwing an exception, we fire a new event (example: kernel.resource_not_found). This gives a greater flexibility and allows us to do the following with listeners:

• prio 0: 404 handler > throws a 404 exception to handle the current kernel.exception, default as it is now
• prio 10: 403 handler, checks permissions on that route (^/admin for example) > throws a 403 exception to deny access
• prio 20: 401 handler, checks authentication > throws a 401 exception to deny access
• My custom Listener: prio 1, initialize symfony1, return content or continue on 404.

This solves a couple of things for symfony as you won't have your dependency issue between the routing and security. It will also allow for stronger fallbacks which additionally, for my personal case, we can use our (by symfony2 defined) error pages if symfony1 also goes into 404 because we can still access the proper chain. Right now we listen to kernel.exception which already is going exit in symfony2.

As far as I can tell, this is only an internal change. The 404 exception will still be thrown at lowest priority and go into the kernel.exception state, thus not causing a BC break. The only possible BC break I can imagine is the additional dependency on the event dispatcher.

I hope I managed to make clear what my idea is, if you have any questions, feel free to ask.

@iltar triggering an event would not prevent other logic from running, while you might not be in a state where you want it to run (and all places wanting to trigger a 404 would then have to be able to return a Response making its way all the way up to the kernel).

@stof but in this issue, that behaviour is actually desired. You can still throw the 404 exception, this won't be backwards incompatible. The event would also trigger the 404 exception (rather than setting content in the event).

The only difference is that using an event, you can hook into it and do some custom checks, for example alter the status from 404 to 403 or 401, without BC breaks.

@iltar if you register a listener on the kernel.exception event with a higher priority than the the ExceptionListener rendering the error page, you can already alter things. this is how the Firewall handles the AccessDeniedException, by redirecting to the login page or replacing the exception with an AccessDeniedHttpException (which will render a 403 in the default listener) depending on whether you are authenticated or no

@wouterj What's the status of this PR? I think it's not worth to invest too much into this.

@fabpot this can't be done with the current code design. I kept this PR open because there is an interesting discussion going on about how to fix this issue. I think it does make a lot of sense to at least try to fix this before Symfony 3.0.

@wouterj "this can't be done with the current code design." Sure, that's why I didn't fix it by then. So, closing now.