Make WordPress Core

Opened 5 weeks ago

Last modified 9 days ago

#60352 new defect (bug)

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

Reported by: azaozz's profile azaozz Owned by:
Milestone: 6.6 Priority: normal
Severity: normal Version: 5.5
Component: General Keywords: needs-patch early
Focuses: Cc:

Description

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.

Change History (4)

#1 @azaozz
5 weeks ago

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.
  • 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.
Last edited 5 weeks ago by azaozz (previous) (diff)

#2 follow-up: @gziolo
4 weeks ago

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

#3 in reply to: ↑ 2 @azaozz
4 weeks ago

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.

#4 @swissspidy
9 days ago

  • Keywords early added
  • Milestone changed from 6.5 to 6.6
Note: See TracTickets for help on using tickets.