WPTT / WPThemeReview

PHP_CodeSniffer rules (sniffs) to enforce WordPress theme review coding conventions
MIT License
209 stars 37 forks source link

[Implement sniff ?] Prevent path disclosure on add_theme_page ? #18

Closed jrfnl closed 8 years ago

jrfnl commented 8 years ago

Decision needed by Theme Review Board:

There is currently no rule to check for the use of __FILE__ in combination with add_theme_page() which could lead to full path disclosure..

There is already a sniff available in WPCS which will check this - WordPress.VIP.PluginMenuSlug.

Should this sniff be activated for theme reviews ?

Advice: Follow WP VIP's lead in this.

To do:

emiluzelac commented 8 years ago

This path should not be used period and in combination with anything theme related.

I would agree for the rule only if we generalize it.

On Tuesday, July 12, 2016, Juliette notifications@github.com wrote:

Decision needed by Theme Review Board:

There is currently no rule to check for the use of FILE in combinaion with add_theme_page() which could lead to full path disclosure..

There is already a sniff available in WPCS which will check this - WordPress.VIP.PluginMenuSlug.

Should this sniff be activated for theme reviews ?

Advice: Follow WP VIP's lead in this. To do:

  • If agreed this should be a rule - add WordPress.VIP.PluginMenuSlug sniff to the ruleset.
  • Add the rule in the Theme Review handbook to the Requirements page.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/WPTRT/WordPress-Coding-Standards/issues/18, or mute the thread https://github.com/notifications/unsubscribe/ABs6zo3ot50cL6yfxX9DxL6yDgq8DTKvks5qU5qzgaJpZM4JKaSy .

grappler commented 8 years ago

@emiluzelac We can cover not allowing __FILE__ completly in another sniff. https://github.com/WPTRT/WordPress-Coding-Standards/issues/23 I would lean to allow this for now till the other sniff has been implimented.

justintadlock commented 8 years ago

I disagree to a generalized rule on __FILE__. The above, specific rule that addresses a specific issue is what we need to go for. When you make generalized checks, you throw out any legit use cases.

It's like the whole wp_deregister_script() in #21. Blocking wp_deregister_script() altogether throws out legit use cases instead of addressing the actual problem of wp_deregister_script( 'jquery' ).

emiluzelac commented 8 years ago

What is a legit use for it?

On Friday, July 15, 2016, Justin Tadlock notifications@github.com wrote:

I disagree to a generalized rule on FILE. The above, specific rule that addresses a specific issue is what we need to go for. When you make generalized checks, you throw out any legit use cases.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WPTRT/WordPress-Coding-Standards/issues/18#issuecomment-232954725, or mute the thread https://github.com/notifications/unsubscribe-auth/ABs6zss88pilfYVSv5W3GN4VJDgVLkjyks5qV48hgaJpZM4JKaSy .

joyously commented 8 years ago

I use a theme that uses __FILE__ for a template trace when in debug mode, so you can see which template files were used to create the final page. That's very helpful when writing a child theme.

justintadlock commented 8 years ago

What is a legit use for it?

Drop-in libraries often need it for finding correct paths and so on.

The real question though is why discourage the use in the first place? Is there are particular issue? If so, we should address those specifically.

emiluzelac commented 8 years ago

Consistency. Can drop-in libraries not use core paths?

On Fri, Jul 15, 2016 at 3:15 PM, Justin Tadlock notifications@github.com wrote:

What is a legit use for it?

Drop-in libraries often need it for finding correct paths and so on.

The real question though is why discourage the use in the first place? Is there are particular issue? If so, we should address those specifically.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WPTRT/WordPress-Coding-Standards/issues/18#issuecomment-233058905, or mute the thread https://github.com/notifications/unsubscribe-auth/ABs6zs6nXPam2ixPmWvMlw27GEqhSLDzks5qV-pygaJpZM4JKaSy .

Pross commented 8 years ago

Consistency. Can drop-in libraries not use core paths?

Some drop-ins are used in plugins and themes.

jrfnl commented 8 years ago

Is there are particular issue? If so, we should address those specifically.

And in this case there is a particular issue this would address, see the original issue description above.

emiluzelac commented 8 years ago

That makes sense @Pross, thanks :)

grappler commented 8 years ago

As the sniff prevents a security issue I am going to add the check.

grappler commented 8 years ago

PR #19 merged...