Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[HttpKernel] AddControllerResolver::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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedNov 6, 2023
edited
Loading

QA
Branch?6.4
Bug fix?no
New feature?yes
Deprecations?no
Issues-
LicenseMIT

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 callControllerResolver::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 thecontroller_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.

dkarlovi reacted with hooray emoji
@carsonbotcarsonbot added this to the6.4 milestoneNov 6, 2023
@carsonbotcarsonbot changed the titleAddControllerResolver::allowControllers() to define which callables are legit controllers when the_check_controller_is_allowed request attribute is set[HttpKernel] AddControllerResolver::allowControllers() to define which callables are legit controllers when the_check_controller_is_allowed request attribute is setNov 6, 2023
@nicolas-grekasnicolas-grekas added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelNov 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 settingtrue 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
MemberAuthor

@nicolas-grekasnicolas-grekasNov 6, 2023
edited
Loading

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-grekasnicolas-grekas merged commitf0fcc9f intosymfony:6.4Nov 7, 2023
@nicolas-grekasnicolas-grekas deleted the controller-check branchNovember 7, 2023 15:05
This was referencedNov 10, 2023
nicolas-grekas added a commit that referenced this pull requestNov 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       | MITForgotten in#52471Commits-------1f560bc [FrameworkBundle] Add TemplateController to the list of allowed controllers for fragments
@marein
Copy link
Contributor

@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 fromAbstractController. However, it doesn't consider controllers tagged withcontroller.service_arguments, which would require special handling. I'm considering submitting a PR that automatically allows all controllers tagged withcontroller.service_arguments too, because I think that it shouldn't make a difference whether one uses#[AsController] orcontroller.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
MemberAuthor

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

marein reacted with thumbs up emoji

nicolas-grekas added a commit that referenced this pull requestFeb 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       | MITIf 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 requestFeb 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       | MITIf 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 requestMar 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       | MITThe `#[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 requestMar 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       | MITThe `#[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 freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@stofstofstof approved these changes

@chalasrchalasrchalasr approved these changes

Assignees
No one assigned
Labels
FeatureHttpKernel❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze"Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.

5 participants
@nicolas-grekas@marein@stof@chalasr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp