Font Library: add mime type validation for font uploads #53986

madhusudhand · 2023-08-28T13:03:31Z

What?

This adds additional mime type validation for uploaded font files.

Why?

To prevent file uploads without a matching mime type.

How?

It checks file mime type with following list and allows only allowed types.

const ALLOWED_FONT_MIME_TYPES = array(
'otf'   => 'font/otf',
'ttf'   => 'font/ttf',
'woff'  => 'font/woff',
'woff2' => 'font/woff2',
);

Testing Instructions

To make the fonts library backend work you need to add this line in your wp-config.php file:
define( 'FONT_LIBRARY_ENABLE', true );

Test with PHP 8.2 version

open Playground link

Try uploading following supported font formats:

OTF
TTF
WOFF
WOFF2

Test with PHP a version < 8

Change the PHP version to 7.4 or any other < 8 and Test same set of font uploads.

It should work successfully with all versions of PHP.

Related issues:

Fixes: #53576

github-actions · 2023-08-28T13:07:54Z

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ phpunit/tests/data/fonts/DMSans.woff2
❔ phpunit/tests/data/fonts/Merriweather.ttf
❔ phpunit/tests/data/fonts/cooper-hewitt.woff
❔ lib/experimental/fonts/font-library/class-wp-font-family.php
❔ lib/experimental/fonts/font-library/class-wp-font-library.php
❔ phpunit/tests/fonts/font-library/wpFontFamily/base.php
❔ phpunit/tests/fonts/font-library/wpFontFamily/install.php
❔ phpunit/tests/fonts/font-library/wpFontFamily/uninstall.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/installFonts.php

mikachan · 2023-09-11T09:03:58Z

lib/experimental/fonts/font-library/class-wp-font-library.php

+		'ttf'   => PHP_VERSION_ID >= 80112 ? 'font/ttf' : 'application/x-font-ttf',
+		'woff'  => PHP_VERSION_ID >= 80112 ? 'font/woff' : 'application/font-woff',
+		'woff2' => PHP_VERSION_ID >= 80112 ? 'font/woff2' : 'application/font-woff2',


I'm not sure about checking for a specific PHP version here; I can't see an example of this anywhere else in the codebase. I'm wondering if we could create an array of allowed mime types to loop through, rather than using the PHP_VERSION_ID check. Something along the lines of:

Suggested change

'ttf' => PHP_VERSION_ID >= 80112 ? 'font/ttf' : 'application/x-font-ttf',

'woff' => PHP_VERSION_ID >= 80112 ? 'font/woff' : 'application/font-woff',

'woff2' => PHP_VERSION_ID >= 80112 ? 'font/woff2' : 'application/font-woff2',

'ttf' => ['font/ttf', 'application/x-font-ttf'],

'woff' => ['font/woff', 'application/font-woff'],

'woff2' => ['font/woff2', 'application/font-woff2'],

Yeah. I agree it is not a good approach. But the mime check doesn't support multiple values in core. So, we should proceed with this approach until there is support for multiple values.

github-actions · 2023-09-13T12:43:06Z

Flaky tests detected in 4349fb94d75243ce07187bf689d74a6c69f7c220.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6234837952
📝 Reported issues:

[Flaky Test] should partially select with shift + click (@webkit) #53524 in /test/e2e/specs/editor/various/multi-block-selection.spec.js

matiasbenedetto · 2023-09-14T04:49:57Z

I found that there are at least 3 real mime types detected by WordPress core depending on the PHP_VERSION_ID:

70333 -> application/font-sfnt
70133 -> application/x-font-ttf
80102 -> font/sfnt

I added a commit to try that. Not sure if it will work as expected.

pbking · 2023-09-19T03:40:18Z

I tinkered with this all day, and I was ultimately able to get all of the uploads to work on each version, but I haven't yet solidified the logic that puts the correct string based on the correct version. It's a frustrating one it is.

I feel like I'm really close, but ultimately the logic just needs to be tweaked until it works for each version.

Ultimately I don't know exactly which version those details changed in. I was just evaluating on minor release versions (7.2, 7.3, 7.4, 8.0, 8.1, 8.2).

It's close and I should be able to finish this in the morning with a fresh head.

add version check for font mimes update ttf mime

pbking · 2023-09-19T12:16:01Z

I tested all of the mime times in all of the PHP versions that I could and found the following to work:

 *  8.2.10 (80210) : font/otf, font/sfnt, font/woff, font/woff2
 *  8.1.23 (80123) : font/otf, font/sfnt, font/woff, font/woff2
 *  8.0.30 (80030) : font/otf, font/sfnt, application/font-woff, application/font-woff2
 *  7.4.33 (70433) : font/otf, font/sfnt, application/font-woff, application/font-woff2
 *  7.3.33 (70333) : font/otf, application/font-sfnt, application/font-woff, application/font-woff2
 *  7.2 (unable to test)

That gave me the following logic for setting mime time expectations:

	const ALLOWED_FONT_MIME_TYPES = array(
		'otf'   => 'font/otf',
		'ttf'   => PHP_VERSION_ID >= 70400 ? 'font/sfnt' : 'application/font-sfnt',
		'woff'  => PHP_VERSION_ID >= 80112 ? 'font/woff' : 'application/font-woff',
		'woff2' => PHP_VERSION_ID >= 80112 ? 'font/woff2' : 'application/font-woff2',
	);

I wasn't able to test 7.2 (I couldn't get any of my environments to run with that version) but since the minimum version of PHP for WordPress is 7.4 I don't think we should try to make an exception for it.

At the moment the verion id to switch between font/woff and application/font-woff is 80000 but that change was introduced later than 8.0 so I believe the value should be 80112 instead.

madhusudhand · 2023-09-19T14:00:16Z

Thank you @matiasbenedetto @pbking for the help and inputs. Updated the mime values based on right PHP versions. All unit tests are working good with all PHP versions.

pbking

Unit Tests are now passing
Went through all scenarios manually with noted versions of PHP and all working as expected.

I believe this is good to 🚢

matiasbenedetto · 2023-09-19T15:30:46Z

lib/experimental/fonts/font-library/class-wp-font-family.php

+		add_filter( 'upload_mimes', array( 'WP_Font_Library', 'set_allowed_mime_types' ) );
 		add_filter( 'upload_dir', array( 'WP_Font_Library', 'set_upload_dir' ) );
 		$were_assets_written = $this->download_or_move_font_faces( $files );
 		remove_filter( 'upload_dir', array( 'WP_Font_Library', 'set_upload_dir' ) );


Should we remove the filter as we do with the upload_dir? I would believe that yes, because otherwise, we would be enabling that globally and that's not the intention I guess.

I tried removing that and the mime type function is no longer working. I'm not sure that we can.

I believe that we should bring this in as it stands. Testing showed we may need the global filter, but the local filter (get_upload_overrides()) maybe not.

Either way I believe this should be brought in and we can work out if it makes sense to pare it down.

Ok, seems good to go. If the mimes in get_upload_overrides() are not necessary, we could remove them, right? Could you add a follow-up issue to remind us to do that?

Agreed. Removed it here #54647

matiasbenedetto

Nice work, guys. Thanks for diving into detail on this.
LGTM 🚀

ramonjd · 2023-09-20T01:56:04Z

Hi!

I'm seeing a few unit test failures (tests introduced in #54490) failing on trunk since this commit.

I can't quite work out the cause, hoping folks who worked on this PR might know.

Thank you!

There were 3 failures:

1) Tests_Fonts_WpFontFamily_Install::test_should_not_install_duplicate_fontfaces with data set "single unique font face" (array('Inter', 'inter', 'Inter', array(array('Inter', 'italic', '900', 'files0'), array('Inter', 'italic', '900', 'files1'))), array(array('inter1.ttf', 'font/ttf', '/tmp/Inter-4R8JU0.tmp', 0, 123), array('inter1.woff', 'font/woff', '/tmp/Inter-0srPEp.tmp', 0, 123)), array('inter_italic_900.ttf'))
Font directory should contain the same number of files as expected
Failed asserting that actual size 0 matches expected size 1.

/var/www/html/wp-content/plugins/gutenberg/phpunit/tests/fonts/font-library/wpFontFamily/install.php:324

2) Tests_Fonts_WpFontFamily_Install::test_should_not_install_duplicate_fontfaces with data set "multiple unique font faces" (array('Lato', 'lato', 'Lato', array(array('Lato', 'normal', '400', 'files0'), array('Lato', 'normal', '500', 'files1'), array('Lato', 'normal', '500', 'files2'))), array(array('lato1.ttf', 'font/ttf', '/tmp/Lato-fd6FlD.tmp', 0, 123), array('lato2.ttf', 'font/ttf', '/tmp/Lato-gjWILY.tmp', 0, 123), array('lato2.woff', 'font/woff', '/tmp/Lato-0k3dey.tmp', 0, 123)), array('lato_normal_400.ttf', 'lato_normal_500.ttf'))
Font directory should contain the same number of files as expected
Failed asserting that actual size 0 matches expected size 2.

/var/www/html/wp-content/plugins/gutenberg/phpunit/tests/fonts/font-library/wpFontFamily/install.php:324

3) Tests_Fonts_WpFontFamily_Install::test_should_overwrite_fontface_with_different_extension
Font directory should contain the same number of files as expected
Failed asserting that actual size 0 matches expected size 2.

/var/www/html/wp-content/plugins/gutenberg/phpunit/tests/fonts/font-library/wpFontFamily/install.php:519

matiasbenedetto · 2023-09-20T02:25:06Z

It's OK they are failing. It seems related to the mime types of the mock files used in the tests. We need to fix the failing ones. Thanks for flagging it @ramonjd .

madhusudhand · 2023-09-20T05:13:48Z

Thanks for flagging @ramonjd. They are fixed here #54645

ramonjd · 2023-09-20T05:21:15Z

They are fixed here #54645

Thanks for the quick turnaround!! 🙇🏻

ironprogrammer · 2023-09-20T19:19:46Z

Small smoke testing follow-up re: the PHP versions comment from @pbking above:

I wasn't able to test 7.2 (I couldn't get any of my environments to run with that version) but since the minimum version of PHP for WordPress is 7.4 I don't think we should try to make an exception for it.

WordPress 6.3 (and 6.4) still offer support down to PHP 7.0, so PHP < 7.4 is important to test if the version could be a factor.

I used Playground running GB trunk to sanity check uploads in PHP 7.0, 7.1, and 7.2, and things worked as expected ✅.

The files tested included real samples, fakes, and some that could be accidentally included in a drag-n-drop situation (like a font license text file or alternate format):

✅ test-otf.otf
✅ test-ttf.ttf
✅ test-woff.woff
✅ test-woff2.woff2
🚫 fake-woff2.woff2 (really a text file, not allowed)
🚫 nope-eot.eot (not allowed)
🚫 nope-svg.svg (not allowed)
🚫 nope-txt.txt (not allowed)
🚫 really-ttf.txt (ttf with txt extension, but not allowed)

madhusudhand requested review from matiasbenedetto, mikachan and vcanales September 8, 2023 09:30

madhusudhand added the [Type] Feature New feature to highlight in changelogs. label Sep 8, 2023

madhusudhand marked this pull request as ready for review September 8, 2023 09:34

madhusudhand requested a review from spacedmonkey as a code owner September 8, 2023 09:34

mikachan added [Feature] Fonts API Needs PHP backport Needs PHP backport to Core [Feature] Typography Font and typography-related issues and PRs and removed [Feature] Fonts API labels Sep 8, 2023

mikachan reviewed Sep 11, 2023

View reviewed changes

madhusudhand force-pushed the fonts/add-mime-check branch 3 times, most recently from b122d31 to 50c4674 Compare September 13, 2023 12:14

madhusudhand and others added 7 commits September 19, 2023 16:42

add mime type validation for font uploads

ec84551

add version check for font mimes update ttf mime

fix unit tests

fd420aa

adding ttf mime type for PHP 7.3

8c8e8c6

adding ttf mime types for PHP 7

3777f23

fix constant reference

3835728

testing version for php8

4813ea3

format

2298ce3

madhusudhand force-pushed the fonts/add-mime-check branch 2 times, most recently from 79b9bf6 to 4349fb9 Compare September 19, 2023 11:17

add unit tests for woff, woff2 and otf font types

2a0cbc9

madhusudhand force-pushed the fonts/add-mime-check branch from 4349fb9 to 9d0f1e2 Compare September 19, 2023 13:35

fix unit tests

67ab0b2

madhusudhand force-pushed the fonts/add-mime-check branch from 9d0f1e2 to 67ab0b2 Compare September 19, 2023 13:42

pbking approved these changes Sep 19, 2023

View reviewed changes

matiasbenedetto reviewed Sep 19, 2023

View reviewed changes

matiasbenedetto approved these changes Sep 19, 2023

View reviewed changes

matiasbenedetto merged commit 47f56bc into trunk Sep 19, 2023
49 checks passed

matiasbenedetto deleted the fonts/add-mime-check branch September 19, 2023 19:00

github-actions bot added this to the Gutenberg 16.7 milestone Sep 19, 2023

madhusudhand mentioned this pull request Sep 20, 2023

Fix Font library unit tests #54645

Merged

madhusudhand mentioned this pull request Sep 20, 2023

Font Library: remove upload_mimes filter after uploading fonts #54647

Merged

matiasbenedetto mentioned this pull request Sep 23, 2023

Font Library: .otf font files are rejected. #54759

Closed

SiobhyB mentioned this pull request Sep 25, 2023

Plugin: Synchronize PHP changes for WordPress 6.4 release #54177

Closed

86 tasks

youknowriad added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Needs PHP backport Needs PHP backport to Core labels Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Font Library: add mime type validation for font uploads #53986

Font Library: add mime type validation for font uploads #53986

madhusudhand commented Aug 28, 2023 •

edited

github-actions bot commented Aug 28, 2023 •

edited

mikachan Sep 11, 2023

madhusudhand Sep 19, 2023

github-actions bot commented Sep 13, 2023 •

edited

matiasbenedetto commented Sep 14, 2023

pbking commented Sep 19, 2023

pbking commented Sep 19, 2023 •

edited

madhusudhand commented Sep 19, 2023

pbking left a comment

matiasbenedetto Sep 19, 2023

pbking Sep 19, 2023

pbking Sep 19, 2023

matiasbenedetto Sep 19, 2023

madhusudhand Sep 20, 2023

matiasbenedetto left a comment •

edited

ramonjd commented Sep 20, 2023

matiasbenedetto commented Sep 20, 2023 •

edited

madhusudhand commented Sep 20, 2023

ramonjd commented Sep 20, 2023

ironprogrammer commented Sep 20, 2023

Font Library: add mime type validation for font uploads #53986

Font Library: add mime type validation for font uploads #53986

Conversation

madhusudhand commented Aug 28, 2023 • edited

What?

Why?

How?

Testing Instructions

Test with PHP 8.2 version

Test with PHP a version < 8

Related issues:

github-actions bot commented Aug 28, 2023 • edited

mikachan Sep 11, 2023

Choose a reason for hiding this comment

madhusudhand Sep 19, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 13, 2023 • edited

matiasbenedetto commented Sep 14, 2023

pbking commented Sep 19, 2023

pbking commented Sep 19, 2023 • edited

madhusudhand commented Sep 19, 2023

pbking left a comment

Choose a reason for hiding this comment

matiasbenedetto Sep 19, 2023

Choose a reason for hiding this comment

pbking Sep 19, 2023

Choose a reason for hiding this comment

pbking Sep 19, 2023

Choose a reason for hiding this comment

matiasbenedetto Sep 19, 2023

Choose a reason for hiding this comment

madhusudhand Sep 20, 2023

Choose a reason for hiding this comment

matiasbenedetto left a comment • edited

Choose a reason for hiding this comment

ramonjd commented Sep 20, 2023

matiasbenedetto commented Sep 20, 2023 • edited

madhusudhand commented Sep 20, 2023

ramonjd commented Sep 20, 2023

ironprogrammer commented Sep 20, 2023

madhusudhand commented Aug 28, 2023 •

edited

github-actions bot commented Aug 28, 2023 •

edited

github-actions bot commented Sep 13, 2023 •

edited

pbking commented Sep 19, 2023 •

edited

matiasbenedetto left a comment •

edited

matiasbenedetto commented Sep 20, 2023 •

edited