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

Fix naive filtering of uploads directory. #6211

Closed
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
c6844b3
Test naive filtering of uploads directory.
peterwilsoncc Mar 3, 2024
b64bfae
Prevent infinite loop filtering fonts directory. Props @costdev.
peterwilsoncc Mar 3, 2024
c1edfba
Restore naive filtering to rest end point.
peterwilsoncc Mar 3, 2024
a866973
Merge branch 'trunk' into fix/font-filter-infinite-loop
peterwilsoncc Mar 5, 2024
19cc1c7
Thought bubble…
peterwilsoncc Mar 5, 2024
a8f9edf
Merge branch 'trunk' into fix/font-filter-infinite-loop
peterwilsoncc Mar 6, 2024
d8dfbf5
Merge branch 'trunk' into fix/font-filter-infinite-loop
peterwilsoncc Mar 7, 2024
a10dbe4
Improve comment.
peterwilsoncc Mar 7, 2024
2b21c6d
Move the font directory filter registration.
peterwilsoncc Mar 7, 2024
ba51db2
Remove cache option: N/A for fonts.
peterwilsoncc Mar 7, 2024
d0ef78e
Docblocks.
peterwilsoncc Mar 7, 2024
3277002
Improve comment.
peterwilsoncc Mar 7, 2024
db876de
More docblocks.
peterwilsoncc Mar 7, 2024
cec16c6
Use static for closure.
peterwilsoncc Mar 7, 2024
94c576b
Revert "Use static for closure."
peterwilsoncc Mar 7, 2024
eff1763
Font uploads.
peterwilsoncc Mar 8, 2024
48aca49
Dockblock improvements
peterwilsoncc Mar 10, 2024
606aeae
Merge branch 'trunk' into fix/font-filter-infinite-loop
peterwilsoncc Mar 10, 2024
5117eef
Avoid duplicate assertions testing for loop.
peterwilsoncc Mar 10, 2024
4b76a9c
Doc fix.: this not the.
peterwilsoncc Mar 10, 2024
addadc3
Font upload, not upload dir.
peterwilsoncc Mar 10, 2024
c1ac4e3
Trolling?
peterwilsoncc Mar 10, 2024
3221610
Merge branch 'trunk' into fix/font-filter-infinite-loop
peterwilsoncc Mar 11, 2024
1c870cc
Rename function.
peterwilsoncc Mar 11, 2024
1c3b7ac
Filter the font directory late.
peterwilsoncc Mar 11, 2024
fb2cfc3
Document late running hook.
peterwilsoncc Mar 11, 2024
607a73a
Merge branch 'trunk' into fix/font-filter-infinite-loop
peterwilsoncc Mar 11, 2024
41bbeb7
Merge branch 'trunk' into fix/font-filter-infinite-loop
peterwilsoncc Mar 12, 2024
52103f6
Rename function to use plural filters.
peterwilsoncc Mar 12, 2024
21673e5
Remove a a typo.
peterwilsoncc Mar 13, 2024
bf2c2fd
Merge branch 'trunk' into fix/font-filter-infinite-loop
peterwilsoncc Mar 13, 2024
1183132
Run filter at default priority.
peterwilsoncc Mar 13, 2024
f2d5a77
Merge branch 'trunk' into fix/font-filter-infinite-loop
peterwilsoncc Mar 15, 2024
0204b99
Remove out of date comment.
peterwilsoncc Mar 15, 2024
9071275
Merge branch 'trunk' into fix/font-filter-infinite-loop
peterwilsoncc Mar 18, 2024
022941e
Merge branch 'trunk' into fix/font-filter-infinite-loop
peterwilsoncc Mar 21, 2024
eccdbac
Merge default getter and filtering function. Rename.
peterwilsoncc Mar 21, 2024
f9e41b2
Merge branch 'trunk' into fix/font-filter-infinite-loop
peterwilsoncc Mar 21, 2024
22764e3
Rename function. Props @swissspidy.
peterwilsoncc Mar 21, 2024
cf211ae
Document use of filter.
peterwilsoncc Mar 21, 2024
35f8e1b
Add ticket reference
swissspidy Mar 22, 2024
ed46c1d
Merge branch 'trunk' into fix/font-filter-infinite-loop
peterwilsoncc Mar 22, 2024
7721163
Merge branch 'fix/font-filter-infinite-loop' of github.com:peterwilso…
peterwilsoncc Mar 22, 2024
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
2 changes: 2 additions & 0 deletions src/wp-includes/default-filters.php
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,8 @@
add_action( 'deleted_post', '_wp_after_delete_font_family', 10, 2 );
add_action( 'before_delete_post', '_wp_before_delete_font_face', 10, 2 );
add_action( 'init', '_wp_register_default_font_collections' );
// Filter the font upload directory. Runs early to ensure the default directory is applied at priority 10 (default).
add_filter( 'font_dir', 'wp_default_font_dir_filter', 5 );

// Add ignoredHookedBlocks metadata attribute to the template and template part post types.
add_filter( 'rest_pre_insert_wp_template', 'inject_ignored_hooked_blocks_metadata_attributes', 10, 2 );
Expand Down
88 changes: 74 additions & 14 deletions src/wp-includes/fonts.php
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,31 @@ function wp_unregister_font_collection( string $slug ) {
return WP_Font_Library::get_instance()->unregister_font_collection( $slug );
}

/**
* Retrieves font uploads directory information.
*
* Same as wp_font_dir() but "light weight" as it doesn't attempt to create the font uploads directory.
* Intended for use in themes, when only 'basedir' and 'baseurl' are needed, generally in all cases
* when not uploading files.
*
* @since 6.5.0
*
* @see wp_font_dir()
*
* @return array See wp_font_dir() for description.
*/
function wp_get_font_dir() {
return wp_font_dir( false );
}

/**
* Returns an array containing the current fonts upload directory's path and URL.
*
* @since 6.5.0
*
* @param bool $create_dir Optional. Whether to check and create the font uploads directory. Default true.
* @return array $defaults {
* Array of information about the upload directory.
* Array of information about the font upload directory.
*
* @type string $path Base directory and subdirectory or full path to the fonts upload directory.
* @type string $url Base URL and subdirectory or absolute URL to the fonts upload directory.
Expand All @@ -107,31 +125,73 @@ function wp_unregister_font_collection( string $slug ) {
* @type string|false $error False or error message.
* }
*/
function wp_get_font_dir() {
function wp_font_dir( $create_dir = true ) {
/*
* Allow extenders to manipulate the font directory consistently.
*
* Ensures the upload_dir filter is fired both when calling this function
* directly and when the upload directory is filtered in the Font Face
* REST API endpoint.
*/
add_filter( 'upload_dir', 'wp_apply_font_dir_filter' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the need to fallback to the uploads directory this may need to run later (say priority 20) to allow for plugins that filter the uploads directory to an offloader at the default priority, 10. This will avoid an order of operations issue as hooks running at the same priority run in the order they are added.

$font_dir = wp_upload_dir( null, $create_dir, false );
remove_filter( 'upload_dir', 'wp_apply_font_dir_filter' );
return $font_dir;
}

/**
* Applies filters to manipulate the font directory used by the font library.
*
* Wrapper for apply_filters( 'font_dir', $font_dir ).
peterwilsoncc marked this conversation as resolved.
Show resolved Hide resolved
* Does not apply filters if already doing the {@see 'font_dir'} filter.
*
* Allows developers to manipulate the font directory used by the
* font library.
*
* @since 6.5.0
*
* @param string $font_dir The font directory.
* @return string The modified font directory.
*/
function wp_apply_font_dir_filter( $font_dir ) {
costdev marked this conversation as resolved.
Show resolved Hide resolved
if ( doing_filter( 'font_dir' ) ) {
// Avoid an infinite loop.
return $font_dir;
}

return apply_filters( 'font_dir', $font_dir );
}

/**
* Modifies the default font directory.
*
* This function should not be called directly, use wp_get_font_dir() instead.
*
* Modifies the default font directory used by WordPress to the subdirectory fonts
* within the content directory. This function should not be called directly but
* rather applied to the {@see 'font_dir'} hook.
*
* @access private
* @since 6.5.0
*
* @see wp_get_font_dir()
*
* @return array See wp_font_dir() for description.
*/
function wp_default_font_dir_filter() {
peterwilsoncc marked this conversation as resolved.
Show resolved Hide resolved
$site_path = '';
if ( is_multisite() && ! ( is_main_network() && is_main_site() ) ) {
$site_path = '/sites/' . get_current_blog_id();
}

$defaults = array(
return array(
'path' => path_join( WP_CONTENT_DIR, 'fonts' ) . $site_path,
'url' => untrailingslashit( content_url( 'fonts' ) ) . $site_path,
'subdir' => '',
'basedir' => path_join( WP_CONTENT_DIR, 'fonts' ) . $site_path,
'baseurl' => untrailingslashit( content_url( 'fonts' ) ) . $site_path,
'error' => false,
);

/**
* Filters the fonts directory data.
*
* This filter allows developers to modify the fonts directory data.
*
* @since 6.5.0
*
* @param array $defaults The original fonts directory data.
*/
return apply_filters( 'font_dir', $defaults );
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -856,21 +856,7 @@ protected function sanitize_src( $value ) {
*/
protected function handle_font_file_upload( $file ) {
add_filter( 'upload_mimes', array( 'WP_Font_Utils', 'get_allowed_font_mime_types' ) );

/*
* Set the upload directory to the fonts directory.
*
* wp_get_font_dir() contains the 'font_dir' hook, whose callbacks are
* likely to call wp_get_upload_dir().
*
* To avoid an infinite loop, don't hook wp_get_font_dir() to 'upload_dir'.
* Instead, just pass its return value to the 'upload_dir' callback.
*/
$font_dir = wp_get_font_dir();
$set_upload_dir = function () use ( $font_dir ) {
return $font_dir;
};
add_filter( 'upload_dir', $set_upload_dir );
add_filter( 'upload_dir', 'wp_apply_font_dir_filter' );

$overrides = array(
'upload_error_handler' => array( $this, 'handle_font_file_upload_error' ),
Expand All @@ -888,7 +874,7 @@ protected function handle_font_file_upload( $file ) {

$uploaded_file = wp_handle_upload( $file, $overrides );

remove_filter( 'upload_dir', $set_upload_dir );
remove_filter( 'upload_dir', 'wp_apply_font_dir_filter' );
remove_filter( 'upload_mimes', array( 'WP_Font_Utils', 'get_allowed_font_mime_types' ) );

return $uploaded_file;
Expand Down
4 changes: 2 additions & 2 deletions tests/phpunit/tests/fonts/font-library/fontLibraryHooks.php
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,13 @@ protected function upload_font_file( $font_filename ) {
$font_file_path = DIR_TESTDATA . '/fonts/' . $font_filename;

add_filter( 'upload_mimes', array( 'WP_Font_Utils', 'get_allowed_font_mime_types' ) );
add_filter( 'upload_dir', 'wp_get_font_dir' );
add_filter( 'upload_dir', 'wp_apply_font_dir_filter' );
$font_file = wp_upload_bits(
$font_filename,
null,
file_get_contents( $font_file_path )
);
remove_filter( 'upload_dir', 'wp_get_font_dir' );
remove_filter( 'upload_dir', 'wp_apply_font_dir_filter' );
remove_filter( 'upload_mimes', array( 'WP_Font_Utils', 'get_allowed_font_mime_types' ) );

return $font_file;
Expand Down
37 changes: 37 additions & 0 deletions tests/phpunit/tests/fonts/font-library/wpFontsDir.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,41 @@ function set_new_values( $defaults ) {

$this->assertSame( static::$dir_defaults, $font_dir, 'The wp_get_font_dir() method should return the default values.' );
}

public function test_fonts_dir_filters_do_not_trigger_infinite_loop() {
swissspidy marked this conversation as resolved.
Show resolved Hide resolved
/*
* Naive filtering of uploads directory to return font directory.
*
* This emulates the approach a plugin developer may take to
* add the filter when extending the font library functionality.
*/
add_filter( 'upload_dir', 'wp_apply_font_dir_filter' );

add_filter(
'upload_dir',
function ( $upload_dir ) {
peterwilsoncc marked this conversation as resolved.
Show resolved Hide resolved
static $count = 0;
++$count;
// The filter may be applied a couple of times, at five iterations assume an infinite loop.
if ( $count >= 5 ) {
$this->fail( 'Filtering the uploads directory triggered an infinite loop.' );
}
return $upload_dir;
},
5
);

/*
* Filter the font directory to return the uploads directory.
*
* This emulates a moving font files back to the uploads directory due
peterwilsoncc marked this conversation as resolved.
Show resolved Hide resolved
* to file system structure.
*/
add_filter( 'font_dir', 'wp_get_upload_dir' );

wp_get_upload_dir();

// This will never be hit if an infinite loop is triggered.
$this->assertTrue( true );
}
}
Loading