Automattic / VIP-Coding-Standards

PHP_CodeSniffer ruleset to enforce WordPress VIP coding standards.
https://wpvip.com/documentation/how-to-install-php-code-sniffer-for-wordpress-com-vip/
Other
236 stars 40 forks source link

Flag iframe buster html files that already exist in the Adbusters plugin #153

Closed nickdaugherty closed 4 years ago

nickdaugherty commented 6 years ago

These should not be used directly in a theme, as they often have vulnerabilities, which are more easily addressed when using the Adbusters plugin.

GaryJones commented 6 years ago

Reference: Adbusters plugin on GitHub

Adbusters does not appear to be part of the vip-go-mu-plugins list.

Buster list.

@nickdaugherty Can you please give some examples of code you'd expect in themes, that we could look for, and example of code which shouldn't be reported?

GaryJones commented 5 years ago

Closing for lack of feedback. Can be re-opened if feedback arrives and it's still something that should be flagged.

nickdaugherty commented 5 years ago

@GaryJones sorry about that, I must have missed your initial ping.

The files that Adbusters looks for are here:

https://github.com/Automattic/Adbusters/tree/master/templates

Essentially we want to ensure that VIP code is not including those files directly and is instead using the Adbusters plugin. I suppose that would look like submoduling Adbusters into the custom PHPCS check, and comparing the files in templates against the scanned code.

Adbusters is not an mu-plugin, as not every VIP site needs it. But those that do use iframe busters for ad networks should be using it as it makes fixing and finding security bugs much easier.

jrfnl commented 4 years ago

I've had a quick look at this. As it is, the issue isn't very actionable as there are no code-samples or anything.

Am I correct when I draw the following conclusions based on the above + the linked content ? Or am I missing the point completely ?

I suppose that would look like submoduling Adbusters into the custom PHPCS check, and comparing the files in templates against the scanned code.

I'm not sure I understand the reasoning for this: if the file is the same, there is basically no real issue. The problem occurs when the file isn't the same, as the file may have vulnerabilities which have already been addressed in the Adbusters plugin.

The only benefit I could see in doing the file compare is that the sniff could throw an error when the file is verified to be the same, and a warning "needs manual inspection" if the file isn't 100% the same.

I expect that doing the file compare is doable, though would recommend it to be "future scope", i.e. after the initial sniff has been created & added to VIPCS.

The way I envision a file compare, would be to:

If not 100% the same, a text compare could be executed to see if there is any overlap at all. If no overlap, the file may not need to be flagged. If an overlap of more than x% (arbitrary number, tbd), the warning could be an error ?

mjangda commented 4 years ago

I vote to just close this. Adbusters isn't really maintained any more.