[Implement sniff ?] Prevent path disclosure on add_theme_page ?

[jrfnl]

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:

• 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.

[emiluzelac]

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
#18, or mute
the thread
https://github.com/notifications/unsubscribe/ABs6zo3ot50cL6yfxX9DxL6yDgq8DTKvks5qU5qzgaJpZM4JKaSy
.

[grappler]

@emiluzelac We can cover not allowing __FILE__ completly in another sniff. #23 I would lean to allow this for now till the other sniff has been implimented.

[justintadlock]

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]

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
#18 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABs6zss88pilfYVSv5W3GN4VJDgVLkjyks5qV48hgaJpZM4JKaSy
.

[joyously]

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]

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]

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
#18 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABs6zs6nXPam2ixPmWvMlw27GEqhSLDzks5qV-pygaJpZM4JKaSy
.

[Pross]

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

Some drop-ins are used in plugins and themes.

[jrfnl]

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]

That makes sense @Pross, thanks :)

[grappler]

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

[grappler]

PR #19 merged...