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 handling of uploading of font files #6407

Open
wants to merge 22 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
30225fc
Remove use of nested filters.
azaozz Apr 18, 2024
78fc27b
Merge remote-tracking branch 'upstream/trunk' into fix-uploading-of-f…
azaozz Apr 29, 2024
1617d81
Add $upload_dir as param to the font_dir filter
azaozz Apr 29, 2024
48339ed
Move _wp_filter_font_directory() to deprecated.php
azaozz Apr 29, 2024
b583c93
Merge remote-tracking branch 'upstream/trunk' into fix-uploading-of-f…
azaozz May 6, 2024
50e91f4
Merge remote-tracking branch 'upstream/trunk' into fix-uploading-of-f…
azaozz May 16, 2024
abc2610
Merge remote trunk
azaozz May 16, 2024
c4b4eb5
Merge remote-tracking branch 'upstream/trunk' into fix-uploading-of-f…
azaozz May 17, 2024
5c4039b
When creating the fonts dir use $font_dir['path'], not $font_dir['pa…
azaozz May 17, 2024
fe6d3b9
Merge remote-tracking branch 'upstream/trunk' into fix-uploading-of-f…
azaozz May 24, 2024
41ec0e8
Merge remote-tracking branch 'upstream/trunk' into fix-uploading-of-f…
azaozz May 29, 2024
86612b6
Update src/wp-includes/fonts.php
azaozz May 30, 2024
8d9e9d8
Update src/wp-includes/fonts.php
azaozz May 30, 2024
d391259
Update src/wp-admin/includes/file.php
azaozz May 30, 2024
4bad2e3
Docs: Remove @see annotations. Not needed there. Props peterwilsoncc.
azaozz May 31, 2024
076854a
Adjust the inline comment to match the changed code
azaozz May 31, 2024
d8c1cba
Merge remote-tracking branch 'upstream/trunk' into fix-uploading-of-f…
azaozz May 31, 2024
f5b0d01
Add docs and some sanity checks when using custom upload_dir data.
azaozz May 31, 2024
0a21712
Merge remote-tracking branch 'upstream/trunk' into fix-uploading-of-f…
azaozz Jul 8, 2024
95bb189
Merge remote-tracking branch 'upstream/trunk' into fix-uploading-of-f…
azaozz Jul 12, 2024
4908744
Merge remote-tracking branch 'upstream/trunk' into fix-uploading-of-f…
azaozz Jul 13, 2024
22b071c
Merge remote-tracking branch 'upstream/trunk' into fix-uploading-of-f…
azaozz Sep 27, 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
10 changes: 9 additions & 1 deletion src/wp-admin/includes/file.php
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,9 @@ function validate_file_to_edit( $file, $allowed_files = array() ) {
* @type bool $test_size Whether to test that the file size is greater than zero bytes.
* @type bool $test_type Whether to test that the mime type of the file is as expected.
* @type string[] $mimes Array of allowed mime types keyed by their file extension regex.
* @type string[] $upload_dir Array of the uploads directory data as returned by
* {@see wp_upload_dir()} or {@see wp_font_dir()}. If not set the function
* will use {@see wp_upload_dir()}.
azaozz marked this conversation as resolved.
Show resolved Hide resolved
* }
* @param string $time Time formatted in 'yyyy/mm'.
* @param string $action Expected value for `$_POST['action']`.
Expand Down Expand Up @@ -972,11 +975,16 @@ function wp_handle_upload_error( &$file, $message ) {
$type = '';
}

if ( isset( $overrides['upload_dir'] ) ) {
$uploads = $overrides['upload_dir'];
azaozz marked this conversation as resolved.
Show resolved Hide resolved
} else {
$uploads = wp_upload_dir( $time );
}

/*
* A writable uploads dir will pass this test. Again, there's no point
* overriding this one.
*/
$uploads = wp_upload_dir( $time );
if ( ! ( $uploads && false === $uploads['error'] ) ) {
return call_user_func_array( $upload_error_handler, array( &$file, $uploads['error'] ) );
}
Expand Down
17 changes: 17 additions & 0 deletions src/wp-includes/deprecated.php
Original file line number Diff line number Diff line change
Expand Up @@ -6318,3 +6318,20 @@ function wp_render_elements_support( $block_content, $block ) {
_deprecated_function( __FUNCTION__, '6.6.0', 'wp_render_elements_class_name' );
return $block_content;
}

/**
* A callback function for use in the {@see 'upload_dir'} filter.
*
* This function is intended for internal use only and should not be used by plugins and themes.
*
* @since 6.5.0
* @deprecated 6.6.0
* @access private
*
* @param string $font_dir The font directory path.
* @return string The font directory path.
*/
function _wp_filter_font_directory( $font_dir ) {
_deprecated_function( __FUNCTION__, '6.6.0' );
return $font_dir;
}
65 changes: 29 additions & 36 deletions src/wp-includes/fonts.php
Original file line number Diff line number Diff line change
Expand Up @@ -128,44 +128,17 @@ 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_filter_font_directory' );
$font_dir = wp_upload_dir( null, $create_dir, false );
remove_filter( 'upload_dir', '_wp_filter_font_directory' );
return $font_dir;
}

/**
* A callback function for use in the {@see 'upload_dir'} filter.
*
* This function is intended for internal use only and should not be used by plugins and themes.
* Use wp_get_font_dir() instead.
*
* @since 6.5.0
* @access private
*
* @param string $font_dir The font directory.
* @return string The modified font directory.
*/
function _wp_filter_font_directory( $font_dir ) {
if ( doing_filter( 'font_dir' ) ) {
// Avoid an infinite loop.
return $font_dir;
}
// This will also create the /uploads directory if it doesn't exist and $create_dir is true.
$upload_dir = wp_upload_dir( null, $create_dir, false );
azaozz marked this conversation as resolved.
Show resolved Hide resolved

// Default fonts directory settings.
$font_dir = array(
'path' => untrailingslashit( $font_dir['basedir'] ) . '/fonts',
'url' => untrailingslashit( $font_dir['baseurl'] ) . '/fonts',
'path' => untrailingslashit( $upload_dir['basedir'] ) . '/fonts',
'url' => untrailingslashit( $upload_dir['baseurl'] ) . '/fonts',
'subdir' => '',
'basedir' => untrailingslashit( $font_dir['basedir'] ) . '/fonts',
'baseurl' => untrailingslashit( $font_dir['baseurl'] ) . '/fonts',
'error' => false,
'basedir' => untrailingslashit( $upload_dir['basedir'] ) . '/fonts',
'baseurl' => untrailingslashit( $upload_dir['baseurl'] ) . '/fonts',
'error' => $upload_dir['error'],
azaozz marked this conversation as resolved.
Show resolved Hide resolved
);

/**
Expand All @@ -185,8 +158,28 @@ function _wp_filter_font_directory( $font_dir ) {
* @type string $baseurl URL path without subdir.
* @type string|false $error False or error message.
* }
* @param array $upload_dir The uploads directory data as returned by {@see wp_upload_dir()}.
*/
return apply_filters( 'font_dir', $font_dir );
$font_dir = apply_filters( 'font_dir', $font_dir, $upload_dir );

// Do not attempt to create the /fonts directory if there was an error in wp_upload_dir() or after the 'font_dir' filter.
if ( ! empty( $font_dir['error'] ) ) {
return $font_dir;
}

if ( $create_dir ) {
$created = wp_mkdir_p( $font_dir['basedir'] );

if ( false === $created ) {
$font_dir['error'] = sprintf(
/* translators: %s: Directory path. */
__( 'Unable to create directory %s. Is its parent directory writable by the server?' ),
esc_html( $font_dir['basedir'] )
);
}
}

return $font_dir;
}

/**
Expand Down
29 changes: 17 additions & 12 deletions src/wp-includes/functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -2872,6 +2872,9 @@ function _wp_check_existing_file_names( $filename, $files ) {
* @param null|string $deprecated Never used. Set to null.
* @param string $bits File content
* @param string $time Optional. Time formatted in 'yyyy/mm'. Default null.
* @param string[] $upload_dir Optional. Array of the uploads directory data as returned by
* {@see wp_upload_dir()} or {@see wp_font_dir()}. If not set
* the function will use {@see wp_upload_dir()}.
azaozz marked this conversation as resolved.
Show resolved Hide resolved
* @return array {
* Information about the newly-uploaded file.
*
Expand All @@ -2881,7 +2884,7 @@ function _wp_check_existing_file_names( $filename, $files ) {
* @type string|false $error Error message, if there has been an error.
* }
*/
function wp_upload_bits( $name, $deprecated, $bits, $time = null ) {
function wp_upload_bits( $name, $deprecated, $bits, $time = null, $upload_dir = array() ) {
if ( ! empty( $deprecated ) ) {
_deprecated_argument( __FUNCTION__, '2.0.0' );
}
Expand All @@ -2895,10 +2898,12 @@ function wp_upload_bits( $name, $deprecated, $bits, $time = null ) {
return array( 'error' => __( 'Sorry, you are not allowed to upload this file type.' ) );
}

$upload = wp_upload_dir( $time );
if ( empty( $upload_dir ) ) {
$upload_dir = wp_upload_dir( $time );
}

if ( false !== $upload['error'] ) {
return $upload;
if ( false !== $upload_dir['error'] ) {
return $upload_dir;
}

/**
Expand All @@ -2920,18 +2925,18 @@ function wp_upload_bits( $name, $deprecated, $bits, $time = null ) {
)
);
if ( ! is_array( $upload_bits_error ) ) {
$upload['error'] = $upload_bits_error;
return $upload;
$upload_dir['error'] = $upload_bits_error;
return $upload_dir;
}

$filename = wp_unique_filename( $upload['path'], $name );
$filename = wp_unique_filename( $upload_dir['path'], $name );

$new_file = $upload['path'] . "/$filename";
$new_file = $upload_dir['path'] . "/$filename";
if ( ! wp_mkdir_p( dirname( $new_file ) ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function attempts to create the directory but it doesn't appear that _wp_handle_upload() does (correct me if I am wrong).

Given these changes allow developers to define the uploads dir, I think it should either:

  • attempt to create a directory similar to the sideloading function
  • add a new error message if the directory doesn't exist, it can use the same string as in the other functions.

If the second approach is taken then the override docs will need to include a note that the directory needs to be created before attempting an upload.

Copy link
Contributor Author

@azaozz azaozz May 31, 2024

Choose a reason for hiding this comment

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

Good catch! Yes, you're right, there is some repetition in wp_upload_bits() as the directory should be checked or created when calling wp_upload_dir(), and is later checked again and eventually created with wp_mkdir_p(). Thinking perhaps the use of wp_mkdir_p() here is mostly to catch edge cases when the upload_dir filter was used.

On the other hand _wp_handle_upload() relies only on the call to wp_upload_dir() to create the directory or ensure that it exists. That seems to be working well enough, been like that "forever" :)

Frankly I'm a bit unsure which fix would be better here.

  1. As you mention we can add a (well documented) requirement that the directory as passed in $upload_dir param must exist. This would give the developers a bit more freedom in how to check, and how to create it if needed.
  2. As far as I see the other option is to use wp_mkdir_p() to confirm or create the directory, same as in wp_upload_bits(). Think there was a slight difference with how is_dir() works with wrappers (perhaps that was only in older PHP versions) so using it to confirm might not be advisable as it might return a different result.

Which do you think is better?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit unsure which fix would be better here.

Same to be 100% honest, I could go either way too. If the upload handlers attempt to create the directory then it seems redundant that wp_upload|font_dir() does too, but this change would complicate that.

Let's sleep on it and hopefully our unconscious can decide for us overnight.

Copy link
Contributor Author

@azaozz azaozz May 31, 2024

Choose a reason for hiding this comment

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

Let's sleep on it and hopefully our unconscious can decide

Great idea! Unfortunately my subconscious did not take that opportunity :)

Joking aside, I tend to very slightly prefer letting plugins handle creation and confirmation of the uploads directory. That would give them a little more freedom in how to do it and whether and how to cache it. Of course they can just use wp_mkdir_p(). Thinking that checking the type and with is_dir(), and adding a longer description there would be sufficient.

Also tried to figure out if there are any differences between PHPs mkdir() and is_dir() when using wrappers. Think there may have been in the past but seems now there isn't (or at least I wasn't able to find anything).

if ( str_starts_with( $upload['basedir'], ABSPATH ) ) {
$error_path = str_replace( ABSPATH, '', $upload['basedir'] ) . $upload['subdir'];
if ( str_starts_with( $upload_dir['basedir'], ABSPATH ) ) {
$error_path = str_replace( ABSPATH, '', $upload_dir['basedir'] ) . $upload_dir['subdir'];
} else {
$error_path = wp_basename( $upload['basedir'] ) . $upload['subdir'];
$error_path = wp_basename( $upload_dir['basedir'] ) . $upload_dir['subdir'];
}

$message = sprintf(
Expand Down Expand Up @@ -2962,7 +2967,7 @@ function wp_upload_bits( $name, $deprecated, $bits, $time = null ) {
clearstatcache();

// Compute the URL.
$url = $upload['url'] . "/$filename";
$url = $upload_dir['url'] . "/$filename";

if ( is_multisite() ) {
clean_dirsize_cache( $new_file );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -856,15 +856,18 @@ 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' ) );
// Filter the upload directory to return the fonts directory.
add_filter( 'upload_dir', '_wp_filter_font_directory' );
Comment on lines -859 to -860
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the back-compat break I am concerned about. If this approach was desired an enhancement should have been opened in June, 2023 WordPress/gutenberg#52704 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking we have to "agree to disagree" here :)
Don't think this is a back-compat problem but rather a fix to a bug that allows plugins to "behave badly" and disable part of the WP plugins API. Another good reason to fix this is the fact that there are no known plugins that do this "bad behavior".


// This will create the /fonts directory if it doesn't exist.
$font_dir = wp_font_dir();

$overrides = array(
'upload_error_handler' => array( $this, 'handle_font_file_upload_error' ),
// Not testing a form submission.
'test_form' => false,
// Only allow uploading font files for this request.
'mimes' => WP_Font_Utils::get_allowed_font_mime_types(),
// Upload to the /fonts directory
'upload_dir' => $font_dir,
);

// Bypasses is_uploaded_file() when running unit tests.
Expand All @@ -874,7 +877,6 @@ protected function handle_font_file_upload( $file ) {

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

remove_filter( 'upload_dir', '_wp_filter_font_directory' );
remove_filter( 'upload_mimes', array( 'WP_Font_Utils', 'get_allowed_font_mime_types' ) );

return $uploaded_file;
Expand Down
9 changes: 6 additions & 3 deletions tests/phpunit/tests/fonts/font-library/fontLibraryHooks.php
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,16 @@ 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_filter_font_directory' );
$font_dir = wp_get_font_dir();

$font_file = wp_upload_bits(
$font_filename,
null,
file_get_contents( $font_file_path )
file_get_contents( $font_file_path ),
null,
$font_dir
);
remove_filter( 'upload_dir', '_wp_filter_font_directory' );

remove_filter( 'upload_mimes', array( 'WP_Font_Utils', 'get_allowed_font_mime_types' ) );

return $font_file;
Expand Down
8 changes: 0 additions & 8 deletions tests/phpunit/tests/fonts/font-library/wpFontsDir.php
Original file line number Diff line number Diff line change
Expand Up @@ -111,14 +111,6 @@ function set_new_values( $defaults ) {
* @ticket 60652
*/
public function test_fonts_dir_filters_do_not_trigger_infinite_loop() {
/*
* 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_filter_font_directory' );

add_filter(
'upload_dir',
function ( $upload_dir ) {
Expand Down
Loading