WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.56k stars 487 forks source link

Check for ABSPATH exit #1850

Open kkmuffme opened 4 years ago

kkmuffme commented 4 years ago

Is your feature request related to a problem?

To avoid direct file access, a lot of WP files disallow access if ABSPATH is not defined. Since this essentially useful for all files that are used in the WP ecosystem, there should be a sniff for this (& perhaps even an auto-fix)

Describe the solution you'd like

if a file does not contain:

if ( ! defined( 'ABSPATH' ) ) {
    exit; // Exit if accessed directly
}

before any other PHP code, EXCEPT if the file contains a require_once for wp-load (which would load the ABSPATH), it should give an error.

jrfnl commented 4 years ago

Actually, the defined() check is essentially a nonsense-check unless the file gives direct output.

So for things like views or the main plugin file this can be considered good practice.

But for files which only contain classes or function definitions, it's a bit nonsensical as no code is being run until it is actively called from somewhere else.

kkmuffme commented 4 years ago

absolutely agree.

However it will be (nearly) impossible to check if a file generates output (or calls a function that generates output) with phpcs I think, which is why I suggested to put it in all cases. (since the output may come from any included/required/loaded via template file; so unless we are happy with just checking in that file, I suggest to generally do it, since there isn't really any drawback to it, is there?)

jrfnl commented 4 years ago

@kkmuffme Well, I don't think promoting nonsensical checks/bad practices is something which we should add to a coding standard.

If at all, a sniff for this should only trigger a warning and only when there is code outside of function/class definitions (and should disregard namespace/use statements).

GaryJones commented 4 years ago

There are also variants on which constants (or functions) are checked to determine if the file has been loaded in a WP context - ABSPATH is not the only way folks check.

remcotolsma commented 1 year ago

The new plugin review team also seems to check for this via PHPCS, the following was part of a review:

Including PHPCS Scan

We are attaching a PHPCS scan result of your plugin to assist you in debugging.

No tool can promise or deliver a 100% security check of your code, and there are issues that PHPCS will flag that we won't block your plugin on, they remain important to review and understand. Security is an ever-evolving concept, and part of our daily development practice requires us to be flexible and adaptive to the changes as they come. Things we thought last year, or even last week, were safe may not be.


Allowing Direct File Access to plugin files

Direct file access is when someone directly queries your file. This can be done by simply entering the complete path to the file in the URL bar of the browser but can also be done by doing a POST request directly to the file. For files that only contain a PHP class the risk of something funky happening when directly accessed is pretty small. For files that contain procedural code, functions and function calls, the chance of security risks is a lot bigger.

You can avoid this by putting this code at the top of all php files:

if ( ! defined( 'ABSPATH' ) ) exit; // Exit if accessed directly

Example(s) from your plugin:

...

... out of a total of 41 coincidences.

I asked in https://wordpress.slack.com/ #pluginreview if this sniff is available somewhere and received the following response from @frantorres:

This check was internally developed and there are plans to publish this check in a future in a plugin to help plugin authors. It looks for any PHP file that does not have that IF structure (that one and derivates) and includes code out of abstractions (class, functions, etc) that could potentially be executed if accessed directly.

https://wordpress.slack.com/archives/C1LBM36LC/p1692695272604879

It may be added to https://github.com/WordPress/plugin-check, if anyone wants to follow up on this, it might be interesting to talk to the plugin review team 💪.

GaryJones commented 1 year ago

While Francisco is correct (and it matches what Juliette said), I'm concerned that the advice of "You can avoid this by putting this code at the top of all php files" (me emphasis) is suggesting a bad practice. Ideally, this would be combined with a check to see that a file either contains a class/trait/interface only OR it has procedural/globally scoped code, and the ABSPATH guard is only added (and advised to be added) to the later.

jrfnl commented 1 year ago

@remcotolsma I think it would be useful for the community if such checks were not scattered across different repos, so the plugin team may want to consider contributing to WPCS...

frantorres commented 1 year ago

Hi, @remcotolsma thanks for collecting all the information and push it to be part of PHPCS.

Also thanks to @GaryJones , I've already changed the message to make it more specific in that line.

You can avoid this by putting this code at the top of all php files that could potentially execute code if accessed directly:

Hi @jrfnl and congrats for the new PHPCS version and your contribution to it, I'm eager to try it. We've been working fast on some automations to make the team work faster on the reviews, I did this internal check and, as for now, sadly I don't have either the time or the knowledge to make it to PHPCS, but I'll be really happy to contribute, and I think improving and maintaining PHPCS would be so good for the plugins team.

GaryJones commented 1 year ago

@frantorres

at the top of all php files

->

at the top of all PHP files

frantorres commented 1 year ago

@GaryJones Thanks for the tip, done!

davidperezgar commented 2 months ago

Thanks for the suggestion! Let's try to contribute, as we are more people in the team. We are new to make new sniff, some help would be welcomed!