Fix the architectural design of `/wp-includes/blocks/index.php`

[azaozz]

Follow-up to #55067.

Generally the *.php files in all "include" directories are meant to be "included" in other files as the name suggests, not loaded directly. This is true for /wp-includes too. In addition these include-only files should not contain any "loose" PHP code that runs in the global scope, only functions and classes. These are pretty simple and easy to follow architectural PHP rules that ensure all code works well and avoid some exploit vectors.

However /wp-includes/blocks/index.php doesn't seem to be following them, similarly to many of the other files in the blocks directory. As far as I see this should be considered as a PHP code architecture bug and should be fixed. The changes should ensure that there is no output or errors when that file is loaded directly.

[azaozz]

I'm unsure if /wp-includes/blocks/index.php or any of the other files in /wp-includes/blocks/ were meant to be accessible by the web server and be loaded directly in the browser. Seems no, but these seem written in a way that would suggests this? As far as I see these files should only be included on REST requests, but that need confirmation.

There are other design inconsistencies in that file:

• It doesn't seem to make sense to do define( 'BLOCKS_PATH', ABSPATH . WPINC . '/blocks/' ); there. It throws errors when ABSPATH or WPINC are not defined. Also most constants in WP are defined in default-constants.php or in /wp-load.php, don't see a reason why this one is not defined there.

• Despite that BLOCKS_PATH is defined when require-dynamic-blocks.php is loaded, the constant is not used in the latter: ​https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/blocks/require-dynamic-blocks.php. Doesn't seem to make sense to define it and not use it?

• Ideally there will be no PHP code that runs in the global scope in any of the files in /wp-includes. This is true for nearly all of them, except for files in /wp-includes/blocks. Thinking the code in these files need to be fixed to follow the basic design principles of the rest of WP.
  • If they must be "entry points" they should check if WP has been bootstrapped and bootstrap it if not.
  • If they were intended only for use in other files, or only for use in the REST API, the loose/global code in them (including calls to add_action(), add_filter(), etc.) should be wrapped in conditionals ensuring these run only when appropriate.

[gziolo]

/wp-includes/blocks/index.php - this file is included as all other files as it registers core blocks that need to be available both in the admin and on the front end. I don't see any reason why it would get executed independently from WordPress core and it definitely was never designed for such purposes.

Despite that BLOCKS_PATH is defined when require-dynamic-blocks.php is loaded

Interesting, this file is auto-generated with a script, so we could easily change it to use BLOCKS_PATH if that makes sense.

[azaozz]

Replying to gziolo:

I don't see any reason why it would get executed independently from WordPress core

Thanks for confirming! Yep, that's what I thought too, this seems like something left over from long ago. What made me doubt it a bit is that the file name is index.php which generally means it is intended for loafing directly in the browser (default behaviour for all web servers afaik).

Interesting, this file is auto-generated with a script, so we could easily change it to use BLOCKS_PATH if that makes sense.

Yea, thinking it can use BLOCKS_PATH especially after defining it is moved to default-constants.php. Thinking that's the best place for it. Then it would make more sense to use this constant everywhere imho.

[nhrrob]

Hi @azaozz we have reviewed this ticket in today's bug scrub.
As it has early keyword and 6.6 beta phase is very near
do you think it can stay in the milestone?

Please share your feedback.
Thank you.

Fix direct accessibility of of /wp-includes/blocks/index.php

Trac ticket: #60352

[oglekler]

@swissspidy are you sure that this ticket need an early keyword? If so, we have to move it to the next milestone, but I am personally cannot image how this simple fix can lead to complications, because it needs ABSPATH to work anyway, otherwise we are getting Fatal error.

[swissspidy]

It's not a simple fix, the provided PR does not really address the reported issue. But I defer to the ticket reporter.

[peterwilsoncc]

I've bumped this from the 6.7 milestone as there hasn't been any progress and the release is passing the early stage.

[peterwilsoncc]

@azaozz I've been re-thinking this, do you think it's worth putting the ​linked pull request, which dies gracefully for direct access, in as an interim step due to the use of index.php?

The remaining issues can still be solved later but at least it will reduce noise in log files?

[azaozz]

Replying to peterwilsoncc:

I've been re-thinking this
...
The remaining issues can still be solved later but at least it will reduce noise in log files?

Sure, makes sense.

As mention in ticket description there is already a PR #6635

Wrong one 😓

[peterwilsoncc]

In 59117:

Editor: Prevent direct access to /wp-includes/blocks/index.php.

Adds a check for ABSPATH to the top of the /wp-includes/blocks/index.php file and prevents the file from loading if it is not defined.

This prevents the file from throwing errors when accessed directly.

Props khokansardar, mukesh27.
Fixes #62108.
See #60352.

Merged r59117 / ​https://github.com/WordPress/wordpress-develop/commit/7e30d693ae42f805da1bd9cac6231db48129223c