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

Escape output sniff fix for static method calls #2370

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

dingo-d
Copy link
Member

@dingo-d dingo-d commented Aug 21, 2023

As it currently is the following code

echo ClassName::someMethodName( $attributes, $test );

Would trigger the EscapeOutput sniff for the class name, but also for any attributes of the class method, except the first one.

The sniff should just trigger the class name, and bow out after that (what is inside the method is of no concern, as the output is all that matters).

I've added a test case and found the part that caused the issue (the *::class part didn't take into account static methods, only the *::class case).

This should fix it.

Adresses part of #1176

@jrfnl
Copy link
Member

jrfnl commented Aug 21, 2023

The sniff should just trigger the class name, and bow out after that (what is inside the method is of no concern, as the output is all that matters).

Is that true though ?

Shouldn't the sniff skip over the method call, but then continue ?

Test case:

echo My::method($param), $somethingElse; // I'd expect two errors here, one for the method call, one for $somethingElse.

@dingo-d
Copy link
Member Author

dingo-d commented Aug 21, 2023

Knew there must be edge cases I missed 😅

I'll work on this some more in the following days, I need to get some sleep 🙈

@jrfnl
Copy link
Member

jrfnl commented Aug 22, 2023

@dingo-d Should this PR be moved back to draft for the time being ?

@dingo-d dingo-d marked this pull request as draft August 22, 2023 07:09
@dingo-d dingo-d marked this pull request as ready for review August 28, 2023 16:57
@dingo-d
Copy link
Member Author

dingo-d commented Aug 28, 2023

@jrfnl The tests seem to be passing now, only the 2 seem to be stalling (or just taking an unusually long time).

Need to write a recursive method that will check the fully qualified class names and if they have a static method call in them. We should also be careful not to catch the throw Exception cases, as for those we do want to check the parameters of the static method if they are escaped or not.
Add check for static public properties, enums and constants.
Add a way to identify static methods in the is_escaping_function, used for setting the customEscapingFunctions.
Split the merge function to merge lowercased method/function names, because PHP is case insensitive for all class, namespace and method names.
@jrfnl jrfnl removed this from the 3.x Next milestone Dec 3, 2023
@dingo-d
Copy link
Member Author

dingo-d commented Dec 11, 2023

Any chance we can move this PR along?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants