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.

Sign up for GitHub

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

(Try) Template Loader: Allow PHP in block templates and parts #25316

Closed
wants to merge 1 commit into from
Closed

(Try) Template Loader: Allow PHP in block templates and parts #25316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/edit-site-export.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function gutenberg_edit_site_export() {
while ( $template_query->have_posts() ) {
$template = $template_query->next_post();
$zip->addFromString(
'theme/block-templates/' . $template->post_name . '.html',
'theme/block-templates/' . $template->post_name . '.php',
gutenberg_strip_post_ids_from_template_part_blocks( $template->post_content )
);
}
Expand All @@ -68,7 +68,7 @@ function gutenberg_edit_site_export() {
while ( $template_part_query->have_posts() ) {
$template_part = $template_part_query->next_post();
$zip->addFromString(
'theme/block-template-parts/' . $template_part->post_name . '.html',
'theme/block-template-parts/' . $template_part->post_name . '.php',
gutenberg_strip_post_ids_from_template_part_blocks( $template_part->post_content )
);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/full-site-editing.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
* @return boolean Whether the current theme is an FSE theme or not.
*/
function gutenberg_is_fse_theme() {
return is_readable( get_stylesheet_directory() . '/block-templates/index.html' );
return is_readable( get_stylesheet_directory() . '/block-templates/index.php' );
}

/**
Expand Down
10 changes: 10 additions & 0 deletions lib/render-script.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php
/**
* Script used to render a block based template passed as argument.
*
* @package gutenberg
*/

// Load the L10n library here.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not sure how to access these functions here.

We'd also need to validate $argv[1] here and makes sure it's file under the block-templates or template parts folder.

Copy link
Contributor

@tomjn tomjn Nov 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be used to execute arbitrary files on the server, it's a major security hole.

Even with whitelisting, people could still hit this file directly in the browser to output the original templates and bypass what the user has set, actings as a means of exposing unintended data as well as other methods of exploiting the feature


require $argv[1];
23 changes: 17 additions & 6 deletions lib/templates-sync.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,18 @@
* @package gutenberg
*/

/**
* Render a Block Based template and returns its output.
*
* @access private
* @internal
*
* @param string $template_path Template file path.
*/
function _gutenberg_read_template( $template_path ) {
return shell_exec( 'php ' . __DIR__ . '/render-script.php ' . $template_path );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should ensure that the new process uses the current PHP interpreter, not just the system's default php. We can do this with the PHP_BINARY constant as of 5.4:

Suggested change
return shell_exec( 'php ' . __DIR__ . '/render-script.php ' . $template_path );
return shell_exec( PHP_BINARY . ' ' . __DIR__ . '/render-script.php ' . $template_path );

Copy link
Contributor

@tomjn tomjn Nov 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have deep misgivings about this approach of executing a new PHP process, it's encouraging developers to bootstrap WordPress to gain access to WP API functions, making these files extremely fragile and easy to break. This also means a big performance hit.

There are also servers that don't allow shell_exec for security reasons

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a thought, aside from a whitelist, it would be good to add 2 extra checks:

  • A check to confirm this is being ran directly rather than via a browser request
  • A nonce to confirm that it is WordPress running the file and not some other piece of code

It may also be worth looking into using an additional parameter to override ini values and prevent the use of networking and file writing functions

}

/**
* Creates a template (or template part depending on the post type)
* auto-draft if it doesn't exist yet.
Expand Down Expand Up @@ -78,7 +90,7 @@ function _gutenberg_get_template_paths( $base_directory ) {
$path_list = array();
if ( file_exists( $base_directory ) ) {
$nested_files = new RecursiveIteratorIterator( new RecursiveDirectoryIterator( $base_directory ) );
$nested_html_files = new RegexIterator( $nested_files, '/^.+\.html$/i', RecursiveRegexIterator::GET_MATCH );
$nested_html_files = new RegexIterator( $nested_files, '/^.+\.php$/i', RecursiveRegexIterator::GET_MATCH );
foreach ( $nested_html_files as $path => $file ) {
$path_list[] = $path;
}
Expand Down Expand Up @@ -134,14 +146,13 @@ function _gutenberg_synchronize_theme_templates( $template_type ) {
$path = $template_file['path'];
$theme = $template_file['theme'];
$template_base_path = $template_base_paths[ $template_type ];

$content = file_get_contents( $path );
$slug = substr(
$content = _gutenberg_read_template( $path );
$slug = substr(
$path,
// Starting position of slug.
strpos( $path, $template_base_path . DIRECTORY_SEPARATOR ) + 1 + strlen( $template_base_path ),
// Subtract ending '.html'.
-5
// Subtract ending '.php'.
-4
Comment on lines +154 to +155
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we program this? e.g.

$suffix = '.php';
$suffix_length = strlen( $suffix );
// ...
strpos( $path, $..., -$suffix_length );

);
_gutenberg_create_auto_draft_for_template( $template_post_types[ $template_type ], $slug, $theme, $content );
}
Expand Down
19 changes: 0 additions & 19 deletions lib/templates.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,25 +5,6 @@
* @package gutenberg
*/

/**
* Returns all block template file path of the current theme and its parent theme.
* Includes demo block template files if demo experiment is enabled.
*
* @return array $block_template_files A list of paths to all template files.
*/
function gutenberg_get_template_paths() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function is useless now.

$block_template_files = glob( get_stylesheet_directory() . '/block-templates/*.html' );
$block_template_files = is_array( $block_template_files ) ? $block_template_files : array();

if ( is_child_theme() ) {
$child_block_template_files = glob( get_template_directory() . '/block-templates/*.html' );
$child_block_template_files = is_array( $child_block_template_files ) ? $child_block_template_files : array();
$block_template_files = array_merge( $block_template_files, $child_block_template_files );
}

return $block_template_files;
}

/**
* Registers block editor 'wp_template' post type.
*/
Expand Down
4 changes: 2 additions & 2 deletions packages/block-library/src/template-part/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ function render_block_core_template_part( $attributes ) {
} else {
// Else, if the template part was provided by the active theme,
// render the corresponding file content.
$template_part_file_path = get_stylesheet_directory() . '/block-template-parts/' . $attributes['slug'] . '.html';
$template_part_file_path = get_stylesheet_directory() . '/block-template-parts/' . $attributes['slug'] . '.php';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generalising template and template-part file access would be good, so that we don't depend on updating these paths and suffixes in multiple places.

if ( 0 === validate_file( $template_part_file_path ) && file_exists( $template_part_file_path ) ) {
$content = file_get_contents( $template_part_file_path );
$content = _gutenberg_read_template( $template_part_file_path );
}
}
}
Expand Down