-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpKernel] Add ControllerResolver::allowControllers()
to define which callables are legit controllers when the _check_controller_is_allowed
request attribute is set
#52471
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
ControllerResolver::allowControllers()
to define which callables are legit controllers when the _check_controller_is_allowed
request attribute is setControllerResolver::allowControllers()
to define which callables are legit controllers when the _check_controller_is_allowed
request attribute is set
…hich callables are legit controllers when the `_check_controller_is_allowed` request attribute is set
a5ae03d
to
893aba9
Compare
$name = preg_replace_callback('/[a-zA-Z_\x7f-\xff][\\\\a-zA-Z0-9_\x7f-\xff]*+@anonymous\x00.*?\.php(?:0x?|:[0-9]++\$)[0-9a-fA-F]++/', fn ($m) => class_exists($m[0], false) ? (get_parent_class($m[0]) ?: key(class_implements($m[0])) ?: 'class').'@anonymous' : $m[0], $name); | ||
} | ||
|
||
if (-1 === $request->attributes->get('_check_controller_is_allowed')) { |
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.
I'm wondering whether we should add the special value -1
here to trigger the deprecation (while setting true
directly already triggers the exception) instead of actually doing what the deprecation says by throwing the exception only in 7.0 (and not having this special -1
value)
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.
I think we need a way for ppl to opt into the new behavior provided by the attribute right in 6.4. But for FragmentListener, we need a way to trigger a deprecation. That's why I made it this way.
…lowed controllers for fragments (nicolas-grekas) This PR was merged into the 6.4 branch. Discussion ---------- [FrameworkBundle] Add TemplateController to the list of allowed controllers for fragments | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #52655 | License | MIT Forgotten in #52471 Commits ------- 1f560bc [FrameworkBundle] Add TemplateController to the list of allowed controllers for fragments
@nicolas-grekas Great feature :). I've a question regarding it's implementation and I thought it's a good place to ask here first. The current functionality automatically allows controllers with the |
I don't know why it's not supported already but I'd happily review a PR that does so. |
…r (marein) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [HttpKernel] Allow tagged controllers in ControllerResolver | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #52471 | License | MIT If the request attribute `_check_controller_is_allowed` is set, only `TemplateController`, children of `AbstractController` and controllers that have the attribute `#[AsController]` are allowed by default. This change also adds controllers tagged with `controller.service_arguments` to the mix. It allows them to be used with different fragment renderers (e.g. `esi` and `ssi`) without having to add any of the above conditions. This pull request is a follow up of [this discussion](#52471 (comment)). Commits ------- 7d2eb5a [HttpKernel] Allow tagged controllers in ControllerResolver
…r (marein) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [HttpKernel] Allow tagged controllers in ControllerResolver | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #52471 | License | MIT If the request attribute `_check_controller_is_allowed` is set, only `TemplateController`, children of `AbstractController` and controllers that have the attribute `#[AsController]` are allowed by default. This change also adds controllers tagged with `controller.service_arguments` to the mix. It allows them to be used with different fragment renderers (e.g. `esi` and `ssi`) without having to add any of the above conditions. This pull request is a follow up of [this discussion](symfony/symfony#52471 (comment)). Commits ------- 7d2eb5a0e8 [HttpKernel] Allow tagged controllers in ControllerResolver
…ute]` attribute (GromNaN) This PR was merged into the 7.3 branch. Discussion ---------- [FrameworkBundle] Enable controller service with `#[Route]` attribute | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | - | License | MIT The `#[AsController]` attribute has 2 purposes: - Add the tag ` controller.service_argument` that configures the service and service argument injection ([RegisterControllerArgumentLocatorsPass](https://github.com/symfony/symfony/blob/79ea49c772ce4b39f414cde5648ad347c3bbcfd7/src/Symfony/Component/HttpKernel/DependencyInjection/RegisterControllerArgumentLocatorsPass.php#L56)) - Allow the class in the controller resolver ([#52471](#52471)) In this PR, I propose to add the tag `argument_resolver.service` on services when the class has the `#[Route]` attribute. Removing the need for `#[AsController]` on classes that use the `#[Route]` attribute. I assume that if there is a route, it is a controller. Diff (from the [docs](https://symfony.com/doc/7.2/controller/service.html)): ```diff namespace App\Controller; use Symfony\Component\HttpFoundation\Response; - use Symfony\Component\HttpKernel\Attribute\AsController; use Symfony\Component\Routing\Attribute\Route; - #[AsController] class HelloController { #[Route('/hello', name: 'hello', methods: ['GET'])] public function index(): Response { // ... } } ``` Commits ------- 2e59467 Enable controller service with #[Route] attribute instead of #[AsController]
…ute]` attribute (GromNaN) This PR was merged into the 7.3 branch. Discussion ---------- [FrameworkBundle] Enable controller service with `#[Route]` attribute | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | - | License | MIT The `#[AsController]` attribute has 2 purposes: - Add the tag ` controller.service_argument` that configures the service and service argument injection ([RegisterControllerArgumentLocatorsPass](https://github.com/symfony/symfony/blob/79ea49c772ce4b39f414cde5648ad347c3bbcfd7/src/Symfony/Component/HttpKernel/DependencyInjection/RegisterControllerArgumentLocatorsPass.php#L56)) - Allow the class in the controller resolver ([#52471](symfony/symfony#52471)) In this PR, I propose to add the tag `argument_resolver.service` on services when the class has the `#[Route]` attribute. Removing the need for `#[AsController]` on classes that use the `#[Route]` attribute. I assume that if there is a route, it is a controller. Diff (from the [docs](https://symfony.com/doc/7.2/controller/service.html)): ```diff namespace App\Controller; use Symfony\Component\HttpFoundation\Response; - use Symfony\Component\HttpKernel\Attribute\AsController; use Symfony\Component\Routing\Attribute\Route; - #[AsController] class HelloController { #[Route('/hello', name: 'hello', methods: ['GET'])] public function index(): Response { // ... } } ``` Commits ------- 2e594672f64 Enable controller service with #[Route] attribute instead of #[AsController]
Right now, when one doesn't configure properly their APP_SECRET, this can too easily lead to an RCE.
This PR proposes to harden security by rejecting any not-allowed controllers when the
_check_controller_is_allowed
request attribute is set. We leverage this in FragmentListener to close the RCE gap.In order to allow a controller, one should call
ControllerResolver::allowControllers()
during instantiation to tell which types or attributes should be accepted. #[AsController] is always allowed, and FrameworkBundle also allows instances ofAbstractController
.Third-party bundles that provide controllers meant to be used as fragments should ensure their controllers are allowed by adding the method call to the
controller_resolver
service definition.I propose this as a late 6.4 feature so that we can provide this hardening right away in 7.0. In 6.4, this would be only a deprecation.