Skip to content

[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

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 6, 2023

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Issues -
License MIT

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 of AbstractController.

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.

@carsonbot carsonbot added this to the 6.4 milestone Nov 6, 2023
@carsonbot carsonbot changed the title Add ControllerResolver::allowControllers() to define which callables are legit controllers when the _check_controller_is_allowed request attribute is set [HttpKernel] Add ControllerResolver::allowControllers() to define which callables are legit controllers when the _check_controller_is_allowed request attribute is set Nov 6, 2023
@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Nov 6, 2023
…hich callables are legit controllers when the `_check_controller_is_allowed` request attribute is set
$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')) {
Copy link
Member

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)

Copy link
Member Author

@nicolas-grekas nicolas-grekas Nov 6, 2023

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.

@nicolas-grekas nicolas-grekas merged commit f0fcc9f into symfony:6.4 Nov 7, 2023
@nicolas-grekas nicolas-grekas deleted the controller-check branch November 7, 2023 15:05
This was referenced Nov 10, 2023
nicolas-grekas added a commit that referenced this pull request Nov 20, 2023
…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
@marein
Copy link
Contributor

marein commented Feb 12, 2024

@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 #[AsController] argument or those that inherit from AbstractController. However, it doesn't consider controllers tagged with controller.service_arguments, which would require special handling. I'm considering submitting a PR that automatically allows all controllers tagged with controller.service_arguments too, because I think that it shouldn't make a difference whether one uses #[AsController] or controller.service_arguments. However, I want to ensure I'm not overlooking any considerations you might have had before proceeding. Looking forward to your feedback :).

@nicolas-grekas
Copy link
Member Author

I don't know why it's not supported already but I'd happily review a PR that does so.

nicolas-grekas added a commit that referenced this pull request Feb 26, 2024
…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
symfony-splitter pushed a commit to symfony/http-kernel that referenced this pull request Feb 26, 2024
…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
fabpot added a commit that referenced this pull request Mar 29, 2025
…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]
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Mar 29, 2025
…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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature HttpKernel ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants