WordPress-Coding-Standards icon indicating copy to clipboard operation
WordPress-Coding-Standards copied to clipboard

Check for ABSPATH exit

Open kkmuffme opened this issue 5 years ago • 11 comments

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.

kkmuffme avatar Jan 03 '20 14:01 kkmuffme

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.

jrfnl avatar Jan 03 '20 15:01 jrfnl

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?)

kkmuffme avatar Jan 03 '20 15:01 kkmuffme

@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).

jrfnl avatar Jan 03 '20 15:01 jrfnl

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.

GaryJones avatar Jan 07 '20 16:01 GaryJones

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.

  • https://github.com/squizlabs/PHP_CodeSniffer
  • https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards

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

remcotolsma avatar Aug 23 '23 07:08 remcotolsma

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.

GaryJones avatar Aug 23 '23 13:08 GaryJones

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

jrfnl avatar Aug 23 '23 13:08 jrfnl

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.

frantorres avatar Aug 25 '23 18:08 frantorres

@frantorres

at the top of all php files

->

at the top of all PHP files

GaryJones avatar Aug 25 '23 18:08 GaryJones

@GaryJones Thanks for the tip, done!

frantorres avatar Aug 26 '23 08:08 frantorres