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

[SecurityBundle] correct types for default arguments for firewall configs #34080

Merged
merged 1 commit into from Oct 28, 2019

Conversation

@shieldo
Copy link
Contributor

shieldo commented Oct 23, 2019

Q A
Branch? 3.4 (and forward)
Bug fix? yes
New feature? no
Deprecations? no
Tickets n/a
License MIT
Doc PR n/a

Up until now, the default template arguments in the security.firewall.config abstract service definition have been each defined (aside from the argument for $listeners which is given a collection type) in the XML as

<argument />

which resolves to an empty string, despite that some of the arguments are typed to being either bool or array|null on the Symfony\Bundle\SecurityBundle\Security\FirewallConfig class itself.

This wouldn't be so much of a problem if the child definitions that use this as a template overrode all the arguments every time, but in the case of firewall configs that mark security as not being enabled, only the first few arguments are overwritten, so firewall config objects that do not have security enabled are instantiated by the DI container with parameters with some of the wrong types.

In general this wouldn't be an issue, as firewalls with security not enabled would not usually be consumed in a context where further security-related config were needed, but there is a case in Symfony\Bundle\SecurityBundle\DataCollector\SecurityDataCollector where the method getSwitchUser() on the firewall config object can be called without checking first whether the firewall has security enabled, which leads to an exception being thrown:

Symfony\Component\Debug\Exception\ContextErrorException
Warning: Illegal string offset 'parameter'
in vendor/symfony/symfony/src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php (line 184) 

which is down to the firewall config being set with an empty string rather than null (in which case the logic here would function as expected).

It seemed most appropriate as a fix (especially given possible introduction of scalar type hints in the future) to apply types to the default arguments so that it was no longer possible to instantiate a firewall config object with parameters of unexpected types.

…figs
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 23, 2019
@nicolas-grekas nicolas-grekas changed the title [SecurityBundle] correct types for default arguments for firewall con… [SecurityBundle] correct types for default arguments for firewall configs Oct 23, 2019
@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Oct 28, 2019

Good catch, thanks @shieldo.

chalasr added a commit that referenced this pull request Oct 28, 2019
…irewall configs (shieldo)

This PR was merged into the 3.4 branch.

Discussion
----------

[SecurityBundle] correct types for default arguments for firewall configs

| Q             | A
| ------------- | ---
| Branch?       | 3.4 (and forward)
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | n/a
| License       | MIT
| Doc PR        | n/a

Up until now, the default template arguments in the `security.firewall.config` abstract service definition have been each defined (aside from the argument for `$listeners` which is given a `collection` type) in the XML as

```xml
<argument />
```

which resolves to an empty string, despite that some of the arguments are typed to being either `bool` or `array|null` on the `Symfony\Bundle\SecurityBundle\Security\FirewallConfig` class itself.

This wouldn't be so much of a problem if the child definitions that use this as a template overrode all the arguments every time, but in the case of firewall configs that mark security as _not_ being enabled, [only the first few arguments are overwritten](https://github.com/symfony/symfony/blob/3.4/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php#L349-L352), so firewall config objects that do not have security enabled are instantiated by the DI container with parameters with some of the wrong types.

In general this wouldn't be an issue, as firewalls with security not enabled would not usually be consumed in a context where further security-related config were needed, but there is a case in `Symfony\Bundle\SecurityBundle\DataCollector\SecurityDataCollector` where the method `getSwitchUser()` on the firewall config object [can be called](https://github.com/symfony/symfony/blob/3.4/src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php#L181) without checking first whether the firewall has security enabled, which leads to an exception being thrown:

```
Symfony\Component\Debug\Exception\ContextErrorException
Warning: Illegal string offset 'parameter'
in vendor/symfony/symfony/src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php (line 184)
```

which is down to the firewall config being set with an empty string rather than `null` (in which case the logic here would function as expected).

It seemed most appropriate as a fix (especially given possible introduction of scalar type hints in the future) to apply types to the default arguments so that it was no longer possible to instantiate a firewall config object with parameters of unexpected types.

<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Always add tests and ensure they pass.
 - Never break backward compatibility (see https://symfony.com/bc).
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too.)
 - Features and deprecations must be submitted against branch 4.4.
 - Legacy code removals go to the master branch.
-->

Commits
-------

6b7044f [SecurityBundle] correct types for default arguments for firewall configs
@chalasr chalasr merged commit 6b7044f into symfony:3.4 Oct 28, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@shieldo shieldo deleted the shieldo:firewall_config_default_args branch Oct 28, 2019
This was referenced Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.