Make WordPress Core

Opened 5 months ago

Last modified 2 weeks 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.7 Priority: normal
Severity: normal Version: 5.5
Component: General Keywords: early has-patch
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 (9)

#1 @azaozz
5 months 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 months ago by azaozz (previous) (diff)

#2 follow-up: @gziolo
5 months 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
5 months 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
4 months ago

  • Keywords early added
  • Milestone changed from 6.5 to 6.6

This ticket was mentioned in Slack in #core by nhrrob. View the logs.


5 weeks ago

#6 @nhrrob
5 weeks ago

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.

This ticket was mentioned in PR #6635 on WordPress/wordpress-develop by @khokansardar.


4 weeks ago
#7

  • Keywords has-patch added; needs-patch removed

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

Trac ticket: #60352

#8 @oglekler
2 weeks ago

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

#9 @swissspidy
2 weeks ago

  • Milestone changed from 6.6 to 6.7

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

Note: See TracTickets for help on using tickets.