WordPress / WordPress-Coding-Standards

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

Empty index.php #451

Closed dmtrmrv closed 1 year ago

dmtrmrv commented 9 years ago

I have several directories in my plugin and add an empty index.php to each of them for some extra security. Something like this:

<?php // Silence is golden.

However this triggers an error:

----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | You must use "/**" style comments for a file comment
   |       | (Squiz.Commenting.FileComment.WrongStyle)
----------------------------------------------------------------------

I was wondering what would be the best way to fix it? Thanks.

JDGrimes commented 9 years ago

You could just exclude all index.php files in your custom XML config file:

 <exclude-pattern>*/index.php</exclude-pattern>
GaryJones commented 9 years ago

I personally think the practice of empty index.php files is wrong. Leave it to the server configuration to hide directory listings.

jrfnl commented 9 years ago

While that would be the proper solution, unfortunately the 'average' WP user doesn't even know what "server configuration" is... so I'd advocate for requiring these empty index files.

GaryJones commented 9 years ago

We're not talking about an average user. We're talking about someone who is creating a Plugin or Theme, and thinks they need an empty index.php file. Those people can be educated. It's a pastable snippet of a line or two in. htacess.

JDGrimes commented 9 years ago

I agree with @GaryJones that it usually doesn't make sense to include index.php files in every directory in an open source project. They are only needed in directories which need to be protected for some reason.

However, on the other hand, WordPress core does continue to include index.php files in every directory, so perhaps we should make provision for that in the WordPress-Core ruleset, at least.

Also, note that using .htaccess is only for servers that are using Apache. I don't know if other options like Nginx are commonly misconfigured in this way, or not, but if so, adding .htaccess really doesn't give complete protection, especially since Nginx is becoming more popular.

ntwb commented 9 years ago

The other workaround it is to include a valid file comment in your index.php files of course:

<?php

/**
 * Do not modify the files in this folder.
 */

And just to add that indeed it is not the "average user" creating the plugin or theme but once finished and if the plugin or theme is distributed on w.org then the "average user" returns to the equation. :smirk:

GaryJones commented 9 years ago

As much as I dislike the use of empty index.php, if they are to exist, then I agree that:

<?php
/** 
 * Intentionally empty file.
 *
 * It exists to stop directory listings on poorly configured servers.
 */

@Drewapicture: Is this something we could decide upon and get into the Handbook, and fixed up in core, so it could then be checked against (if it exists) for plugins and themes?

GaryJones commented 9 years ago

There's already a ticket open for standardising file headers in core, so I've commented there.

JDGrimes commented 8 years ago

I'm rethinking this. According to the handbook:

Whenever possible, all WordPress files should contain a header DocBlock, regardless of the file’s contents – this includes files containing classes. [emphasis in original]

Above I said this:

However, on the other hand, WordPress core does continue to include index.php files in every directory, so perhaps we should make provision for that in the WordPress-Core ruleset, at least.

It turns out that it isn't true. The only directories that include index.php files without any code or file level docblocks are those that don't contain bundled source code:

/wp-content/
/wp-content/plugins/
/wp-content/themes/

All of the the other directories that contain an index.php file are actually using them, that is, they contain code, and are accompanied by a file-level docblock.

So I suggest that we don't make special allowances for index.php files. WordPress can either update these files to have proper headers, or perhaps just exclude those directories from sniffing. Proposing we close this as wontfix.

ntwb commented 7 years ago

In the meantime I'm going to stick the template hierarchy in an index.php file or two 😏

https://johnblackbourn.com/ascii-wordpress-template-hierarchy

GaryJones commented 7 years ago

Pinging @DrewAPicture for an opinion.

DrewAPicture commented 7 years ago

@GaryJones Which part specifically are you seeking an opinion on? I can get on board with adding file headers to the empty indexes purely for standardization sake if that's what you're asking.

DrewAPicture commented 7 years ago

@GaryJones Also, I disagree with your assertion that we aren't in fact talking about average users. The whole point of the indexes is to prevent listing on poorly configured user servers, which is where the plugins are going to live eventually. So it's absolutely about average users not even knowing their servers are configured poorly.

GaryJones commented 7 years ago

I can get on board with adding file headers to the empty indexes purely for standardization sake if that's what you're asking.

Yes, that's it entirely. Checking for the existence and conformance of file headers then won't have to be skipped for the three index.php files.

The whole point of the indexes is to prevent listing on poorly configured user servers, which is where the plugins are going to live eventually.

And yet most of the directories in WP don't have them?

It's not even a safe workaround, as it's trivial for a server to have a different DirectoryIndex (or Nginx equivalent)

DrewAPicture commented 7 years ago

OK, to prevent listing content. And again, DirectoryIndex/Nginx/[insert confusing jargon here] is irrelevant to the users that are actually going to be hosting the plugins.

GaryJones commented 7 years ago

OK, to prevent listing content.

That makes sense. It's easy enough for a bad actor to find out what a site is likely to have in their wp-includes, but less so for their wp-content.

And again, DirectoryIndex/Nginx/[insert confusing jargon here] is irrelevant to the users that are actually going to be hosting the plugins.

I think we're talking about different aspects here, but it's irrelevant to the point in hand.

Question: Would you want the Handbook to advise plugin and theme developers to use index.php files in their directories?

Question: Is the previous example code a) sufficient to meet the WPCS and WP Doc standards, and b) suitably descriptive wording?

<?php
/** 
 * Intentionally empty file.
 *
 * It exists to stop directory listings on poorly configured servers.
 */
JDGrimes commented 7 years ago

Question: Would you want the Handbook to advise plugin and theme developers to use index.php files in their directories?

No directories within the plugin should contain user content, because they'll be overwritten by updates. So this wouldn't be for the directories within the plugin, but directories that the plugin creates in /wp-content for storing content in.

KhairulA commented 4 years ago

I use this in the index.php file, hope it helps:

<?php // phpcs:ignore PEAR.Commenting.FileComment.Missing
// Silence is golden.
AndrzejLan commented 3 years ago

Ok. I agree with empty index.php. PHP manuals teach how to fill this file with code. I can't understand how a program should run when index.php is empty.

dingo-d commented 3 years ago

@AndrzejLan You should check out this amazing diagram Rarst made on wpse https://wordpress.stackexchange.com/a/26622/58895

index.php in a plugin is not the same as the one in the WordPress core. When the web request comes to your WP app the index.php file in the WordPress core will get 'pinged', and then the app execution will happen. The WP app will then execute the rest of the code, plugins included.

smileBeda commented 3 years ago

Even with the workaround mentioned here https://github.com/WordPress/WordPress-Coding-Standards/issues/451#issuecomment-139961024 (which is the only correct thing to do, removing index files is not adequate as others have already expressed here too), we still get one warning, which is the "there must be exactly one blank line after the file comment"

That warning only disappears if after that blank line actually comes something EOF is not accepted as something valid and thus, it seems to trigger that warning, no matter what.

So, this triggers no notice:

<?php
/**
 * Intentionally empty file.
 *
 * It exists to stop directory listings on poorly configured servers.
 *
 * @package           whatever
 */

echo 'this';

But this, does:

<?php
/**
 * Intentionally empty file.
 *
 * It exists to stop directory listings on poorly configured servers.
 *
 * @package           whatever
 */

This is even happening after running the beautifier.

Thus, I think there is something we can improve here, which is, don't assume there should be some code after a file comment

Is this possible? Does this need a new ticket?

jrfnl commented 3 years ago

@TukuToi The sniff used for this is not a WPCS native sniff, but one from PHPCS itself, so if you want to open an issue about it, it should be done upstream.

As a plugin/theme developer, you're probably best off to ignore these empty index.php files from within your custom ruleset using this snippet:

    <!-- Exclude the 'empty' index files from some documentation checks -->
    <rule ref="Squiz.Commenting.FileComment">
        <exclude-pattern>*/index\.php</exclude-pattern>
    </rule>
    <rule ref="Squiz.Commenting.InlineComment.NoSpaceBefore">
        <exclude-pattern>*/index\.php</exclude-pattern>
    </rule>

... for when the file content is something like this:

<?php
/**
 * Nothing to see here.
 */

Or this code snippet:

    <!-- Exclude the 'empty' index files from some documentation checks -->
    <rule ref="Squiz.Commenting.FileComment">
        <exclude-pattern>*/index\.php</exclude-pattern>
    </rule>

... for when the file content is something like the index.php file you posted above.

Mind: this does presume you have no index.php files with actual code in your project! If you do, you'll need to adjust the exclude patterns to make sure those will still get scanned.

smileBeda commented 3 years ago

Thanks @jrfnl - since I also have theme code, that is my least preferred solution. I have reported this upstream, sorry I didn't realise that this is a PHP Sniffer issue rather than a WP Sniffer issue. Thanks!

jrfnl commented 3 years ago

@TukuToi No worries. FYI: The CONTRIBUTING.md doc has information about how to determine whether an issue is a WPCS issue or an upstream one.

smileBeda commented 2 years ago

This last issue has now been resolved in PHP_CodeSniffer 3.6.1 according https://github.com/squizlabs/PHP_CodeSniffer/issues/3384#issuecomment-882127645

3.6.1 was released October 11th 2021.

Thus every issue within this ticket is resolved - I guess it could be "closed" (even if it is a question and not a actionable task on WPCS side).