Font Library: run wp_font_family post content sanitization using wp_insert_post_data filter

[matiasbenedetto]

What?

This PR changes where and when data sanitization happens, running it right before it is inserted into the database using the wp_insert_post_data filter.

Why?

This is done for security reasons, to ensure that we are not storing data that is unescaped.
The wp_font_family posts could theoretically be created outside of the context of the Font Library class, e.g. via manual insertion of a post, and currently we are only calling the sanitization method inside of the Font Library class.

How?

Calling the sanitization function on wp_font_family type posts to filter the data before insertion into the database.

Testing Instructions

1. Try to save a wp_font_family post with inappropriate content.
2. Check if the data was sanitized.

Marking this as a draft because it requires #53273 to work as expected.

[github-actions]

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

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

[github-actions]

Flaky tests detected in 770c5b5.
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/7213572544
📝 Reported issues:

• [Flaky Test] should display the "Manage Fonts" icon #56776 in /test/e2e/specs/site-editor/font-library.spec.js

[mikachan]

Thanks for picking this up, @jffng! I've left a few comments, they're mainly around "should we add a check for this".

[jffng]

@mikachan thanks for the feedback, I think I've addressed it all.

[mikachan]

Nice, the changes look good to me!

[jffng]

I have one more concern with this PR:

The wp_insert_post_data filter is called every time a post of any type is created, updated, or deleted. I ran a search of the core codebase, and it's only used four times: https://github.com/search?q=repo%3AWordPress%2Fwordpress-develop+add_filter%28+%27wp_insert_post_data%27+&type=code

I'm wondering if this is an efficient way to go about sanitization, or is it overkill?

[matiasbenedetto]

The wp_insert_post_data filter is called every time a post of any type is created, updated, or deleted. I ran a search of the core codebase, and it's only used four times: https://github.com/search?q=repo%3AWordPress%2Fwordpress-develop+add_filter%28+%27wp_insert_post_data%27+&type=code

I'm wondering if this is an efficient way to go about sanitization, or is it overkill?

The reason to use wp_insert_post_data is that we need to make sure we are running the sanitization over the JSON content stored in the database no matter how the wp_font_family post type is created (using the customized API endpoints or not) to avoid potential risky content saved and after that rendered to the pages. If you are aware of other ways to do the same, we can explore it. Otherwise, I would stick to this.

[matiasbenedetto]

After discussing it again with @jffng, we could close this PR. It was created on the belief that we could create posts of custom post types using the default endpoint, but that's not the case, Right now, I can't think of a way to create a wp_font_family post bypassing our custom endpoint, and, because of that, bypassing the sanitation added by the custom endpoint.

Feel free to re-open if you can think of a way that a malicious user could create a wp_font_family bypassing the custom endpoint and sanitation.