123inkt / php-codesniffer-baseline

PHP_Codesniffer extension to allow baselining existing issues.
MIT License
14 stars 4 forks source link

Issue with several sniffs, which would apply for the same output and the same line #6

Closed norgeindian closed 1 year ago

norgeindian commented 1 year ago

We have the strange issue, that phpcs fails in our pipeline, although we use the exact same command to create the baseline file locally before. But step by step. Locally, we're running the following command in our docker container to create the phpcs.baseline.xml:

vendor/bin/phpcs --warning-severity=7 --standard=dev/tests/static/framework/Magento --exclude=Magento.Annotation.MethodArguments,Magento.Annotation.MethodAnnotationStructure,Magento2.Functions.StaticFunction,Magento2.Exceptions.DirectThrow src --report=\DR\CodeSnifferBaseline\Reports\Baseline --report-file=phpcs.baseline.xml --basepath=/var/www/html/

Running now locally the same check without the baseline creation, we will not face any issues anymore. In the pipeline instead, we're still getting errors. I checked that a bit more detailed and found out, that sometimes several sniffs would apply to a specific code line. In the baseline, there is one sniff, the output shows the other. In our case, these are always the following two:

Locally, we only get Magento2.Security.XssTemplate.FoundNotAllowed, whereas the pipeline still finds issues at the exact same spots. (Not all occurrences of Magento2.Security.XssTemplate.FoundNotAllowed lead to issues, only some). If I replace these few with Magento2.Security.XssTemplate.FoundUnescaped, the check is green. To be honest, I have no idea, if the issue is actually related to this module or if it's the defined standard, which leads to it, but maybe someone has faced sth similar and give me a hint?

peterjaap commented 1 year ago

I'm having the exact same issue! Although in my case, I am already getting it locally.

I run phpcs through grumphp, and I'm seeing Magento2.Security.XssTemplate.FoundNotAllowed there:

$ vendor/bin/grumphp run --tasks phpcs
FILE: ...frontend/Pluim/Hyva/Magento_Newsletter/templates/subscribe.phtml
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 19 | WARNING | Unescaped output detected.
    |         | (Magento2.Security.XssTemplate.FoundNotAllowed)
 21 | WARNING | Unescaped output detected.
    |         | (Magento2.Security.XssTemplate.FoundNotAllowed)
----------------------------------------------------------------------

Even though the baseline adds the Magento2.Security.XssTemplate.FoundUnescaped sniff;

$ cat phpcs.baseline.xml | grep subscribe 
<violation file="app/design/frontend/Pluim/Hyva/Magento_Newsletter/templates/subscribe.phtml" sniff="Magento2.Security.XssTemplate.FoundUnescaped" signature="f0430b643472c3ec4f8e21f6f88170cb34d4b1f6"/>
<violation file="app/design/frontend/Pluim/Hyva/Magento_Newsletter/templates/subscribe.phtml" sniff="Magento2.Security.XssTemplate.FoundUnescaped" signature="f0430b643472c3ec4f8e21f6f88170cb34d4b1f6"/>
peterjaap commented 1 year ago

@norgeindian for your information, I also ran into this issue (amongst others) when I tried https://github.com/isaaceindhoven/php-code-sniffer-baseliner.

I thought it was maybe that package, so I switched to this one (I like the approach this package uses better!), but same error.

peterjaap commented 1 year ago

Maybe it's because there are actually two warnings inside one sniff? This seems to be the only one that has that https://github.com/magento/magento-coding-standard/blob/097bda3e015f35dc7c2efc0b8c7a7d8dfc158a63/Magento2/Sniffs/Security/XssTemplateSniff.php#L331

peterjaap commented 1 year ago

@norgeindian the variable $hasDisallowedAnnotation seems to cause this problem. In certain cases (not sure which yet), this equates to true, in other cases false. This is what causes the other warning to appear.

As a test, I edited the file vendor/magento/magento-coding-standard/Magento2/Sniffs/Security/XssTemplateSniff.php locally and set private $hasDisallowedAnnotation = true; instead of private $hasDisallowedAnnotation = false; and I re-ran both the baseline generator and phpcs (through grumphp) and then it passes:

$ php vendor/bin/phpcs --standard=YouweMagento2 app/code app/design/frontend --report=\\DR\\CodeSnifferBaseline\\Reports\\Baseline --report-file=phpcs.baseline.xml --basepath=.
Time: 37.94 secs; Memory: 102.77MB

$ vendor/bin/grumphp run --tasks phpcs                                                                                                                                          
GrumPHP is sniffing your code!

Running tasks with priority 0!
==============================

Running task 1/1: phpcs... ✔
peterjaap commented 1 year ago

There's already a fix! By the author of the other baseline package :)

See the PR here https://github.com/magento/magento-coding-standard/pull/377

This issue can be closed since it has nothing to do with this package.

norgeindian commented 1 year ago

@peterjaap , thanks for helping here. Then we can indeed close this ticket, as it is purely related to Magento coding standard.