Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax allowed path restrictions for block metadata collections and make the allowed root list filterable #8130

Prev Previous commit
Next Next commit
Revise implementation to use collection roots only as a denylist, fai…
…ling for exact matches and parent directories.
felixarntz committed Jan 16, 2025
commit 63d4c9e2be8c6325f11f91798ac1a9f37263f3f5
75 changes: 40 additions & 35 deletions src/wp-includes/class-wp-block-metadata-registry.php
Original file line number Diff line number Diff line change
@@ -43,7 +43,7 @@ class WP_Block_Metadata_Registry {
* @since 6.8.0
* @var string[]|null
*/
private static $default_allowed_collection_roots = null;
private static $default_collection_roots = null;

/**
* Registers a block metadata collection.
@@ -84,42 +84,48 @@ class WP_Block_Metadata_Registry {
public static function register_collection( $path, $manifest ) {
$path = wp_normalize_path( rtrim( $path, '/' ) );

$allowed_collection_roots = self::get_default_allowed_collection_roots();
$collection_roots = self::get_default_collection_roots();

/**
* Filters in which root directory paths block metadata collections are allowed.
* Filters the root directory paths for block metadata collections.
*
* The default list encompasses the `wp-includes` directory, as well as the root directories for plugins,
* must-use plugins, and themes. This filter can be used to expand the list, e.g. to custom directories with
* symlinked plugins.
* Any block metadata collection that is registered must not use any of these paths, or any parent directory
* path of them. Most commonly, block metadata collections should reside within one of these paths, though in
* some scenarios they may also reside in entirely different directories (e.g. in case of symlinked plugins).
*
* Example:
* * It is allowed to register a collection with path `WP_PLUGIN_DIR . '/my-plugin'`.
* * It is not allowed to register a collection with path `WP_PLUGIN_DIR`.
* * It is not allowed to register a collection with path `dirname( WP_PLUGIN_DIR )`.
*
* Any block metadata collection that is registered must be within one of these directories. It must however
* not match any of these directories exactly, as then the collection may conflict with another one within the
* same root.
* The default list encompasses the `wp-includes` directory, as well as the root directories for plugins,
* must-use plugins, and themes. This filter can be used to expand the list, e.g. to custom directories that
* contain symlinked plugins, so that these root directories cannot be used themselves for a block metadata
* collection either.
*
* @since 6.8.0
*
* @param string[] $allowed_collection_roots List of allowed metadata collection root paths.
* @param string[] $collection_roots List of allowed metadata collection root paths.
*/
$allowed_collection_roots = apply_filters( 'wp_allowed_block_metadata_collection_roots', $allowed_collection_roots );
$collection_roots = apply_filters( 'wp_allowed_block_metadata_collection_roots', $collection_roots );

$allowed_collection_roots = array_unique(
$collection_roots = array_unique(
array_map(
static function ( $allowed_root ) {
return rtrim( $allowed_root, '/' );
},
$allowed_collection_roots
$collection_roots
)
);

// Check if the path is valid:
if ( ! self::is_valid_collection_path( $path, $allowed_collection_roots ) ) {
if ( ! self::is_valid_collection_path( $path, $collection_roots ) ) {
_doing_it_wrong(
__METHOD__,
sprintf(
/* translators: %s: list of allowed collection roots */
__( 'Block metadata collections can only be registered within one of the following directories: %s' ),
esc_html( implode( wp_get_list_item_separator(), $allowed_collection_roots ) )
esc_html( implode( wp_get_list_item_separator(), $collection_roots ) )
),
'6.8.0'
);
@@ -251,46 +257,45 @@ private static function default_identifier_callback( $path ) {
}

/**
* Checks whether the given block metadata collection path is valid against the list of allowed collection roots.
* Checks whether the given block metadata collection path is valid against the list of collection roots.
*
* @since 6.8.0
*
* @param string $path Block metadata collection path, without trailing slash.
* @param string[] $allowed_collection_roots List of allowed collection root paths, without trailing slashes.
* @param string $path Block metadata collection path, without trailing slash.
* @param string[] $collection_roots List of collection root paths, without trailing slashes.
* @return bool True if the path is allowed, false otherwise.
*/
private static function is_valid_collection_path( $path, $allowed_collection_roots ) {
$matching_root_found = false;

foreach ( $allowed_collection_roots as $allowed_root ) {
// If the path matches any allowed root exactly, it is invalid.
private static function is_valid_collection_path( $path, $collection_roots ) {
foreach ( $collection_roots as $allowed_root ) {
// If the path matches any root exactly, it is invalid.
if ( $allowed_root === $path ) {
return false;
}

// Otherwise, if the path is within any of the allowed roots, it is valid.
if ( str_starts_with( $path, $allowed_root ) ) {
$matching_root_found = true;
// If the path is a parent path of any of the roots, it is invalid.
if ( str_starts_with( $allowed_root, $path ) ) {
return false;
}
}

return $matching_root_found;
return true;
}

/**
* Gets the default allowed collection root paths.
* Gets the default collection root directory paths.
*
* @since 6.8.0
*
* @return string[] List of directory paths within which metadata collections are allowed.
*/
private static function get_default_allowed_collection_roots() {
if ( isset( self::$default_allowed_collection_roots ) ) {
return self::$default_allowed_collection_roots;
private static function get_default_collection_roots() {
if ( isset( self::$default_collection_roots ) ) {
return self::$default_collection_roots;
}

$allowed_collection_roots = array(
$collection_roots = array(
wp_normalize_path( ABSPATH . WPINC ),
wp_normalize_path( WP_CONTENT_DIR ),
wp_normalize_path( WPMU_PLUGIN_DIR ),
wp_normalize_path( WP_PLUGIN_DIR ),
);
@@ -300,10 +305,10 @@ private static function get_default_allowed_collection_roots() {
$theme_roots = array( $theme_roots );
}
foreach ( $theme_roots as $theme_root ) {
$allowed_collection_roots[] = trailingslashit( wp_normalize_path( WP_CONTENT_DIR ) ) . ltrim( wp_normalize_path( $theme_root ), '/' );
$collection_roots[] = trailingslashit( wp_normalize_path( WP_CONTENT_DIR ) ) . ltrim( wp_normalize_path( $theme_root ), '/' );
}

self::$default_allowed_collection_roots = array_unique( $allowed_collection_roots );
return self::$default_allowed_collection_roots;
self::$default_collection_roots = array_unique( $collection_roots );
return self::$default_collection_roots;
}
}
84 changes: 80 additions & 4 deletions tests/phpunit/tests/blocks/wpBlockMetadataRegistry.php
Original file line number Diff line number Diff line change
@@ -80,12 +80,88 @@ public function test_register_collection_with_invalid_plugin_path() {
$this->assertFalse( $result, 'Invalid plugin path should not be registered' );
}

public function test_register_collection_with_non_existent_path() {
$non_existent_path = '/path/that/does/not/exist';
public function test_register_collection_with_valid_muplugin_path() {
$plugin_path = WPMU_PLUGIN_DIR . '/my-plugin/blocks';
$result = WP_Block_Metadata_Registry::register_collection( $plugin_path, $this->temp_manifest_file );
$this->assertTrue( $result, 'Valid must-use plugin path should be registered successfully' );
}

public function test_register_collection_with_invalid_muplugin_path() {
$invalid_plugin_path = WPMU_PLUGIN_DIR;

$this->setExpectedIncorrectUsage( 'WP_Block_Metadata_Registry::register_collection' );

$result = WP_Block_Metadata_Registry::register_collection( $invalid_plugin_path, $this->temp_manifest_file );
$this->assertFalse( $result, 'Invalid must-use plugin path should not be registered' );
}

public function test_register_collection_with_valid_theme_path() {
$theme_path = WP_CONTENT_DIR . '/themes/my-theme/blocks';
$result = WP_Block_Metadata_Registry::register_collection( $theme_path, $this->temp_manifest_file );
$this->assertTrue( $result, 'Valid theme path should be registered successfully' );
}

public function test_register_collection_with_invalid_theme_path() {
$invalid_theme_path = WP_CONTENT_DIR . '/themes';

$this->setExpectedIncorrectUsage( 'WP_Block_Metadata_Registry::register_collection' );

$result = WP_Block_Metadata_Registry::register_collection( $invalid_theme_path, $this->temp_manifest_file );
$this->assertFalse( $result, 'Invalid theme path should not be registered' );
}

public function test_register_collection_with_arbitrary_path() {
$arbitrary_path = '/var/arbitrary/path';
$result = WP_Block_Metadata_Registry::register_collection( $arbitrary_path, $this->temp_manifest_file );
$this->assertTrue( $result, 'Arbitrary path should be registered successfully' );
}

public function test_register_collection_with_arbitrary_path_and_collection_roots_filter() {
$arbitrary_path = '/var/arbitrary/path';
add_filter(
'wp_allowed_block_metadata_collection_roots',
static function ( $paths ) use ( $arbitrary_path ) {
$paths[] = $arbitrary_path;
return $paths;
}
);

$this->setExpectedIncorrectUsage( 'WP_Block_Metadata_Registry::register_collection' );

$result = WP_Block_Metadata_Registry::register_collection( $arbitrary_path, $this->temp_manifest_file );
$this->assertFalse( $result, 'Arbitrary path should not be registered if it matches a collection root' );

$result = WP_Block_Metadata_Registry::register_collection( dirname( $arbitrary_path ), $this->temp_manifest_file );
$this->assertFalse( $result, 'Arbitrary path should not be registered if it is a parent directory of a collection root' );

$result = WP_Block_Metadata_Registry::register_collection( $arbitrary_path . '/my-plugin/blocks', $this->temp_manifest_file );
$this->assertTrue( $result, 'Arbitrary path should be registered successfully if it is within a collection root' );
}

public function test_register_collection_with_wp_content_parent_directory_path() {
$invalid_path = dirname( WP_CONTENT_DIR );

$this->setExpectedIncorrectUsage( 'WP_Block_Metadata_Registry::register_collection' );

$result = WP_Block_Metadata_Registry::register_collection( $invalid_path, $this->temp_manifest_file );
$this->assertFalse( $result, 'Invalid path (parent directory of "wp-content") should not be registered' );
}

public function test_register_collection_with_wp_includes_parent_directory_path() {
$invalid_path = ABSPATH;

$this->setExpectedIncorrectUsage( 'WP_Block_Metadata_Registry::register_collection' );

$result = WP_Block_Metadata_Registry::register_collection( $invalid_path, $this->temp_manifest_file );
$this->assertFalse( $result, 'Invalid path (parent directory of "wp-includes") should not be registered' );
}

public function test_register_collection_with_non_existent_manifest() {
$non_existent_manifest = '/path/that/does/not/exist/block-manifest.php';

$this->setExpectedIncorrectUsage( 'WP_Block_Metadata_Registry::register_collection' );

$result = WP_Block_Metadata_Registry::register_collection( $non_existent_path, $this->temp_manifest_file );
$this->assertFalse( $result, 'Non-existent path should not be registered' );
$result = WP_Block_Metadata_Registry::register_collection( '/var/arbitrary/path', $non_existent_manifest );
$this->assertFalse( $result, 'Non-existent manifest should not be registered' );
}
}

Unchanged files with check annotations Beta

await expect(
page,
'should redirect to the installation page'
).toHaveURL( /wp-admin\/install\.php$/ );

Check failure on line 40 in tests/e2e/specs/install.test.js

GitHub Actions / Test with SCRIPT_DEBUG enabled / Run E2E tests

[chromium] › install.test.js:34:6 › WordPress installation process › should install WordPress with pre-existing database credentials

1) [chromium] › install.test.js:34:6 › WordPress installation process › should install WordPress with pre-existing database credentials Error: should redirect to the installation page Timed out 5000ms waiting for expect(locator).toHaveURL(expected) Locator: locator(':root') Expected pattern: /wp-admin\/install\.php$/ Received string: "http://localhost:8889/" Call log: - should redirect to the installation page with timeout 5000ms - waiting for locator(':root') - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" 38 | page, 39 | 'should redirect to the installation page' > 40 | ).toHaveURL( /wp-admin\/install\.php$/ ); | ^ 41 | 42 | await expect( 43 | page.getByText( /WordPress database error/ ), at /home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/install.test.js:40:5

Check failure on line 40 in tests/e2e/specs/install.test.js

GitHub Actions / Test with SCRIPT_DEBUG disabled / Run E2E tests

[chromium] › install.test.js:34:6 › WordPress installation process › should install WordPress with pre-existing database credentials

1) [chromium] › install.test.js:34:6 › WordPress installation process › should install WordPress with pre-existing database credentials Error: should redirect to the installation page Timed out 5000ms waiting for expect(locator).toHaveURL(expected) Locator: locator(':root') Expected pattern: /wp-admin\/install\.php$/ Received string: "http://localhost:8889/" Call log: - should redirect to the installation page with timeout 5000ms - waiting for locator(':root') - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" - locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" 38 | page, 39 | 'should redirect to the installation page' > 40 | ).toHaveURL( /wp-admin\/install\.php$/ ); | ^ 41 | 42 | await expect( 43 | page.getByText( /WordPress database error/ ), at /home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/install.test.js:40:5
await expect(
page.getByText( /WordPress database error/ ),
}
async create(applicationName = TEST_APPLICATION_NAME) {
await this.admin.visitAdminPage( '/profile.php' );

Check failure on line 98 in tests/e2e/specs/profile/applications-passwords.test.js

GitHub Actions / Test with SCRIPT_DEBUG enabled / Run E2E tests

[chromium] › profile/applications-passwords.test.js:19:6 › Manage applications passwords › should correctly create a new application password

2) [chromium] › profile/applications-passwords.test.js:19:6 › Manage applications passwords › should correctly create a new application password Error: Not logged in 96 | 97 | async create(applicationName = TEST_APPLICATION_NAME) { > 98 | await this.admin.visitAdminPage( '/profile.php' ); | ^ 99 | 100 | const newPasswordField = this.page.getByRole( 'textbox', { name: 'New Application Password Name' } ); 101 | await expect( newPasswordField ).toBeVisible(); at Admin.visitAdminPage (/home/runner/work/wordpress-develop/wordpress-develop/node_modules/@wordpress/e2e-test-utils-playwright/src/admin/visit-admin-page.ts:36:9) at ApplicationPasswords.create (/home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/profile/applications-passwords.test.js:98:3) at /home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/profile/applications-passwords.test.js:23:3

Check failure on line 98 in tests/e2e/specs/profile/applications-passwords.test.js

GitHub Actions / Test with SCRIPT_DEBUG enabled / Run E2E tests

[chromium] › profile/applications-passwords.test.js:19:6 › Manage applications passwords › should correctly create a new application password

2) [chromium] › profile/applications-passwords.test.js:19:6 › Manage applications passwords › should correctly create a new application password Retry #1 ─────────────────────────────────────────────────────────────────────────────────────── Error: Not logged in 96 | 97 | async create(applicationName = TEST_APPLICATION_NAME) { > 98 | await this.admin.visitAdminPage( '/profile.php' ); | ^ 99 | 100 | const newPasswordField = this.page.getByRole( 'textbox', { name: 'New Application Password Name' } ); 101 | await expect( newPasswordField ).toBeVisible(); at Admin.visitAdminPage (/home/runner/work/wordpress-develop/wordpress-develop/node_modules/@wordpress/e2e-test-utils-playwright/src/admin/visit-admin-page.ts:36:9) at ApplicationPasswords.create (/home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/profile/applications-passwords.test.js:98:3) at /home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/profile/applications-passwords.test.js:23:3

Check failure on line 98 in tests/e2e/specs/profile/applications-passwords.test.js

GitHub Actions / Test with SCRIPT_DEBUG disabled / Run E2E tests

[chromium] › profile/applications-passwords.test.js:19:6 › Manage applications passwords › should correctly create a new application password

2) [chromium] › profile/applications-passwords.test.js:19:6 › Manage applications passwords › should correctly create a new application password Error: Not logged in 96 | 97 | async create(applicationName = TEST_APPLICATION_NAME) { > 98 | await this.admin.visitAdminPage( '/profile.php' ); | ^ 99 | 100 | const newPasswordField = this.page.getByRole( 'textbox', { name: 'New Application Password Name' } ); 101 | await expect( newPasswordField ).toBeVisible(); at Admin.visitAdminPage (/home/runner/work/wordpress-develop/wordpress-develop/node_modules/@wordpress/e2e-test-utils-playwright/src/admin/visit-admin-page.ts:36:9) at ApplicationPasswords.create (/home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/profile/applications-passwords.test.js:98:3) at /home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/profile/applications-passwords.test.js:23:3

Check failure on line 98 in tests/e2e/specs/profile/applications-passwords.test.js

GitHub Actions / Test with SCRIPT_DEBUG disabled / Run E2E tests

[chromium] › profile/applications-passwords.test.js:19:6 › Manage applications passwords › should correctly create a new application password

2) [chromium] › profile/applications-passwords.test.js:19:6 › Manage applications passwords › should correctly create a new application password Retry #1 ─────────────────────────────────────────────────────────────────────────────────────── Error: Not logged in 96 | 97 | async create(applicationName = TEST_APPLICATION_NAME) { > 98 | await this.admin.visitAdminPage( '/profile.php' ); | ^ 99 | 100 | const newPasswordField = this.page.getByRole( 'textbox', { name: 'New Application Password Name' } ); 101 | await expect( newPasswordField ).toBeVisible(); at Admin.visitAdminPage (/home/runner/work/wordpress-develop/wordpress-develop/node_modules/@wordpress/e2e-test-utils-playwright/src/admin/visit-admin-page.ts:36:9) at ApplicationPasswords.create (/home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/profile/applications-passwords.test.js:98:3) at /home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/profile/applications-passwords.test.js:23:3
const newPasswordField = this.page.getByRole( 'textbox', { name: 'New Application Password Name' } );
await expect( newPasswordField ).toBeVisible();