Skip to content

[Security] Do not use First Class Callable Syntax for listeners #46260

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Security] Do not use First Class Callable Syntax for listeners #46260

wants to merge 1 commit into from

Conversation

javer
Copy link
Contributor

@javer javer commented May 5, 2022

Q A
Branch? 6.1
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

This PR fixes usage of First Class Callable Syntax for registering listeners which are intended to be unregistered later.

I'm targeting 6.1 branch because the issue was introduced only in this branch via b0217c6

The reason of this change is clear from the following example:

<?php

class Foo {
    public function bar() {}
}

$foo = new Foo();
var_dump($foo->bar(...) === $foo->bar(...)); // false
var_dump([$foo, 'bar'] === [$foo, 'bar']);   // true

In other words, it's not safe to use First Class Callable Syntax in the cases when we need to refer to the same function to remove it from somewhere, because each method(...) returns a new function.

@javer javer requested review from wouterj and chalasr as code owners May 5, 2022 11:15
@carsonbot carsonbot added this to the 6.1 milestone May 5, 2022
@carsonbot
Copy link

It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you.

Cheers!

Carsonbot

@javer
Copy link
Contributor Author

javer commented May 5, 2022

Also it seems that CI is broken:

Can't contact LDAP server (-1)
Uncaught UnexpectedValueException: Intersection types not yet supported
Symfony\Bundle\WebProfilerBundle\Tests\Resources\IconTest::testIconFileContents with data set #22516

and so on.

nicolas-grekas
nicolas-grekas reviewed May 5, 2022
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I don't think this is the patch we want to merge. Instead, remove listener should be able to work with first-class callables. I'm going to submit a PR doing so, so that we can discuss about it.

@nicolas-grekas nicolas-grekas mentioned this pull request May 5, 2022
Merged
@nicolas-grekas
Copy link
Member

See #46262

@chalasr
Copy link
Member

chalasr commented May 5, 2022

Closing in favor of #46262 which reuses the test cases you contributed. Thank you.

@chalasr chalasr closed this May 5, 2022
nicolas-grekas added a commit that referenced this pull request May 5, 2022
@nicolas-grekas
bug #46262 [EventDispatcher] Fix removing listeners when using first-…
b368ce1 
…class callable syntax (javer)

This PR was merged into the 4.4 branch.

Discussion
----------

[EventDispatcher] Fix removing listeners when using first-class callable syntax

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #46260
| License       | MIT
| Doc PR        | -

Closures should be compared using non-strict comparison to account for the first-class callable syntax.
See https://github.com/php/php-src/blob/9a90bd705483004c2ef408ee9e9bb0902beade3f/Zend/zend_closures.c#L387-L423

Commits
-------

df4c003 [EventDispatcher] Fix removing listeners when using first-class callable syntax
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof approved these changes

@wouterj wouterj Awaiting requested review from wouterj wouterj is a code owner

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees
No one assigned
Labels
Bug Security Status: Reviewed
Projects
None yet
Milestone
6.1
Development

Successfully merging this pull request may close these issues.
5 participants
@javer @carsonbot @nicolas-grekas @chalasr @stof