Consider allowing themes to register block metadata collections

[flixos90]

The new wp_register_block_metadata_collection() function introduced in #62002 currently only allows registering block metadata folders within plugins or the wp-includes directory (the latter for Core's own usage).

This ticket is about discussing further whether themes should also be able to use the function. This was originally raised by @spacedmonkey in ​https://github.com/WordPress/wordpress-develop/pull/7303#discussion_r1780156397.

At a first glance, here's two conflicting arguments that may have us lean towards yes or no:

• Yes: The other functions related to register block types are not restricted to plugins, so there's no good reason that this new function only allows plugin usage.
• No: Themes should not register block types, so while it's possible to use the functions, this should not happen, so arguably the function should not add theme support.

Curious to get other people's thoughts on this.

[flixos90]

cc @mreishus

[dougwollison]

I want to push for any such restirctions on where a block is registered from to be removed.

First, as someone who builds tailored sites for projects, I always register custom block types within the theme. I mean, thats where I'm already registering custom post types, taxonomies, metadata, and god knows what else, so why not the blocks that are being styled by the theme?

Second, others have pointed out in the duplicate (#62251) that it breaks if the hosting environment uses symlinks.

Third, and most simply, the current restriction prevents mu-plugins from registering collections too. I can't think of any good reason for that to be intentional. If it's updated to allow those too, then at that point why bother whitelisting directory at all for a single variant of the registration API?

[flixos90]

Replying to dougwollison:

I want to push for any such restirctions on where a block is registered from to be removed.

While registering block types in themes is discouraged (and forbidden for .org themes), it's reasonable to expect that it should still be possible, just like how all those other things are possible. So I'm onboard for adding support to themes (and must-use plugins).

If it's updated to allow those too, then at that point why bother whitelisting directory at all for a single variant of the registration API?

The reason those conditions exist has nothing to do with allowlisting in the first place. They exist because we need to prevent certain directories from being used, for example it needs to be impossible to register e.g. the root plugins or root themes directory as a block collection, since then all block types from all other plugins would be considered part of such a collection. So that part of the behavior needs to be maintained no matter what we allowlist. Another example would be if someone tried to register the parent directory of the entire WordPress site, that would be a problem so it needs to be prevented.

[swissspidy]

Re-sharing comments from #62251 / ​https://make.wordpress.org/core/2024/10/17/new-block-type-registration-apis-to-improve-performance-in-wordpress-6-7/

It should be perfectly fine to allow registering block type collections from the theme. At some point, we added the support for individual block types to integrate with themes. In practice, it was possible with WP hooks to register block type in themes, so we decided to make the experience smoother while keep recommending locating block types in plugins.

Agreed. Just because it’s discouraged doesn’t mean it should be forbidden. Themes in the repo won’t be allowed to have blocks in them anyway, and for custom projects developers should have the freedom to do this, even if it’s semantically not the best place.

[swissspidy]

#62251 was marked as a duplicate.

[gziolo]

One of my comments from the duplicate ticket was included as a quote. I believe the following is the best summary of where I would draw the line:

• themes from the official registry shouldn’t contain blocks to avoid the challenge with switching between themes
• custom themes, in particular these built for individual sites based on the specific requirements should not have these restrictions and the collection should be supported the same as individual blocks
• the paths used with individual blocks should be the same as with collections, they should be already restricted to plugins, themes and the location where core blocks live.

[dougwollison]

Replying to flixos90:

The reason those conditions exist has nothing to do with allowlisting in the first place. They exist because we need to prevent certain directories from being used, for example it needs to be impossible to register e.g. the root plugins or root themes directory as a block collection, since then all block types from all other plugins would be considered part of such a collection. So that part of the behavior needs to be maintained no matter what we allowlist. Another example would be if someone tried to register the parent directory of the entire WordPress site, that would be a problem so it needs to be prevented.

Wait, how is that so disasterous? I'm looking at the source again and I'm confused.

From what I can tell, registering a collection simply enables skipping the block.json file reading in register_block_type_from_metadata(). If a match isn't found though, it still reads the specified JSON file anyway, which has no restrictions on where it's trying to read from. There are no glob or recursive scans used to find block files, so registering a root directory doesn't seem like it'd do much damage.

I haven't literally tried it, but if I actually tried registering the root folder as a collection:

<?php
wp_register_block_metadata_collection( ABSPATH, 'my-manifest.php' );

It looks like the absolute worst it could do is:

• Nullify the performance improvement provided by the metadata registry by forcing register_block_type_from_metadata() to just read the json files as before, and/or,
• Allow my manifest file to specify overrides for blocks from core or other plugins, and really slopily at that. If I wanted to mess with that stuff it'd be simpler to hook into block_type_metadata or block_type_metadata_settings.

If we're already on the same page that themes and mu-plugins should be included in the checks, fine, but I'm thrown off by this argument as it doesn't track with my current understanding of how this all works.

[flixos90]

Replying to dougwollison:

Replying to flixos90:

The reason those conditions exist has nothing to do with allowlisting in the first place. They exist because we need to prevent certain directories from being used, for example it needs to be impossible to register e.g. the root plugins or root themes directory as a block collection, since then all block types from all other plugins would be considered part of such a collection. So that part of the behavior needs to be maintained no matter what we allowlist. Another example would be if someone tried to register the parent directory of the entire WordPress site, that would be a problem so it needs to be prevented.

Wait, how is that so disasterous? I'm looking at the source again and I'm confused.

When register_block_type_from_metadata() runs, we check if there's a block metadata collection that encompasses that given file location. For example, if I have a block file wp-content/plugins/my-plugin/blocks/test/block.json, a collection with wp-content/plugins/my-plugin would match that (good), but so would a collection with wp-content/plugins (bad). We wouldn't want to allow someone to tamper with how other plugins' blocks are registered, it could be used to override the block configuration data by other plugins. That might make sense for a project you manage, but it would open up lots of doors for malicious intent to any plugin modifying another plugin's behavior. The risk is greater than the reward for this. Other than that, it's also a bit wasteful for performance because every plugin's blocks would also match that overall wp-content/plugins block metadata collection.

Last but not least, there's efforts like #62267, which would make it even more problematic if it was possible to register block metadata collections for those root directories.

[dougwollison]

Replying to flixos90:

Replying to dougwollison:

Wait, how is that so disasterous? I'm looking at the source again and I'm confused.

We wouldn't want to allow someone to tamper with how other plugins' blocks are registered, it could be used to override the block configuration data by other plugins. That might make sense for a project you manage, but it would open up lots of doors for malicious intent to any plugin modifying another plugin's behavior.

But... core literally provides two separate filters to allow messing with how blocks are registered. The entire premise of the hook system is to allow plugins to modify each other's behaviour, not just core's behaviour. I sincerely can't grasp how this is a uniquely dangerous scenario.

[flixos90]

Replying to dougwollison:

Replying to flixos90:

Replying to dougwollison:

Wait, how is that so disasterous? I'm looking at the source again and I'm confused.

We wouldn't want to allow someone to tamper with how other plugins' blocks are registered, it could be used to override the block configuration data by other plugins. That might make sense for a project you manage, but it would open up lots of doors for malicious intent to any plugin modifying another plugin's behavior.

But... core literally provides two separate filters to allow messing with how blocks are registered. The entire premise of the hook system is to allow plugins to modify each other's behaviour, not just core's behaviour. I sincerely can't grasp how this is a uniquely dangerous scenario.

Still, that's a different kind of API where that is more explicit. To use the new block metadata collection API, you need to create a manifest, which you can reasonably do for your plugin, maybe even a few plugins manually, but you realistically can't create a manifest that works for all blocks from all plugins. Additionally, if it was possible to register a collection for wp-content/plugins, different plugins might just try to override each other. What makes sense for a block type registration filter doesn't necessarily make sense here, due to the problems outlined in how the API is designed.

[dougwollison]

Replying to flixos90:

To use the new block metadata collection API, you need to create a manifest, which you can reasonably do for your plugin, maybe even a few plugins manually, but you realistically can't create a manifest that works for all blocks from all plugins. Additionally, if it was possible to register a collection for wp-content/plugins, different plugins might just try to override each other.

Oh, I wasn't advocating for such an insane use case. I was interpreting this limitation as a security measure (that doesn't secure much), not code-enforced rule of best practice.

If we only open up collection registration /themes/* and /mu-plugins/*, I'm good. I'm only citing other edge cases like symlinked directories. Perhaps a disallow approach would be better, if anything.

[assassinateur]

I want to also raise the issue of symlinked plugins. Managed hosts run their managed plugins (Jetpack, WooCommerce and such) in a symlinked fashion, the bigger the plugin (the one that benefits from this the most) the more likely it will be symlinked. So this is not limited to local dev, but actual large sites. For example, all of WordPress.com run managed plugins, the Woo plan in Pressable runs WooCommerce symlinked.

[bowedk]

I'd love to see this working for themes as well, as I aim to keep all my blocks centralized within the theme rather than split across different plugins. Having everything contained within the theme gives me a more streamlined setup, create child themes, and extend functionality.

Nothing concrete, just a side note :)

[mreishus]

Hey @flixos90 - Wanted to bring up some more real-world scenarios I've come across wrt the path validation.

• Some hosting services used symlinked plugins, like /wp-content/plugins/myplugin to /managed-plugins/myplugin. The current checks fail because the physical path doesn't match the expected plugin directory structure.
• WordPress.com and other complex setups would like to support custom paths like registering from wp-content/mu-plugins/jetpack-plugin.

I've thought about a couple of ideas and have come up with this list of potential approaches:

1. Keep the current structure but add more valid paths, like themes and mu-plugins.
2. Switched to a banned path approach. - Everything is allowed by default, but some paths are explicitly disallowed, like wp-content/plugins, wp-content/themes, or wp-content/ to prevent conflicts. (Maybe we could climb all the way 'up' the directory path and disallow any of these?)
3. Add a WordPress filter for paths being allowed for custom setups. Imagine something like $is_valid = apply_filters( 'block_metadata_collection_path_is_valid', $default_validity, $path );

Each has different tradeoffs, and there could be other ideas I haven't thought of yet. Do you have any inclination on what would balance the original concerns about security and conflicts with some of the real-word needs we've seen?

[desrosj]

Because of a few bug reports opened since 6.7 was released, the Core team is evaluating the need for a short 6.7.1 cycle (possibly next week).

While this issue addresses something introduced in 6.7, it actually feels more like an enhancement (though the last comment from @mreishus seems to point out some issues with this as currently implemented), it's still in the discussion phase and needs more time for collaboration.

With this in mind, I'm going to punt to 6.7.2. But if consensus and the required confidence level is reached before 6.7.1 is shipped, it can be moved back.