Include security sniffs in PHPCS ruleset

[iandunn]

Currently, our custom ruleset includes the sniffs for prepared queries, but not for XSS or CSRF.

I couldn't find any previous discussions about why they're not included. The only thing I can think of is that there might be too many false positives?

In my experience, the XSS sniff works well. The CSRF one sometimes generates false positives, but I think it'd be better to include it, and then refine our code and/or the sniff to address those, than it would be to not use it at all, and take the risk of a vulnerability slipping through.

[netweb]

Sounds good to me 👍🏼

[jrf]

There are two reasons why these sniffs are not included in the core ruleset:

1. The Core CS handbook does not contain any guidelines for this. The core ruleset follows the handbook, so at this moment, the handbook does not justify adding these sniffs.
2. Historically core does not escape any translations. Changing this would IMHO be a positive precedent, but should be discussed more thoroughly.

Also, in my opinion, the nonce verification sniff will need more work before it is suitable to be added for core.

And to give you some insight in the amount issues which would be identified:

• Enabling the XSS sniff would at this moment generate 5500+ errors which would all need to be manually evaluated and, where necessary, fixed.
• Enabling the CSRF sniff would at this moment generate 945+ errors+warnings which would all need to be manually evaluated and, where necessary, fixed.

[iandunn]

Thanks for the background info Juliette! I agree on both #1 and #2.

Assuming #2 doesn't change, though, I'm guessing it'd be relatively easy to make the XSS sniff ignore translated strings? That should cut the 5500 number down quite a bit. If it does change, then I'm assuming phpcbf could fix them automatically.

I don't think adding a bunch of new errors is necessarily a big deal, though, since it's easy to ​filter the phpcs report to only the lines that were modified while working on a patch. Then, we could clean up errors in old code as a dedicated task, without it getting in the way of new work.

What do you think?