Directory

WP_Theme_JSON_Gutenberg: Add nested indexed array schema sanitization by matiasbenedetto · Pull Request #56447 · WordPress/gutenberg · GitHub
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

Jump to bottom

WP_Theme_JSON_Gutenberg: Add nested indexed array schema sanitization #56447

Merged
merged 14 commits into from
Nov 28, 2023
Merged

WP_Theme_JSON_Gutenberg: Add nested indexed array schema sanitization #56447

merged 14 commits into from
Nov 28, 2023

Conversation

matiasbenedetto
Copy link
Contributor

@matiasbenedetto matiasbenedetto commented Nov 22, 2023 •
edited

What?

  • Add nested indexed array schema sanitization capabilities to the WP_Theme_JSON_Gutenberg class.
  • Add settings.typography.fontFamilies validation in WP_Theme_JSON_Gutenberg
  • Add unit tests.

This is an alternative approach to #53273, and hopefully fixes: #52798

Co-authrored by: @jffng

Why?

To sanitize data stored in indexed arrays, for example, font family definitions. Without this PR, that's not possible.

Currently, WP_Theme_JSON sanitization is not able to sanitize data contained on indexed arrays. So certain data from theme.json, for example, settings.typography.fontFamilies which is a JSON array, cannot be sanitized because when parsing the JSON dataWP_Theme_JSON translates JSON arrays into PHP indexed arrays and the class is not capable of sanitizing that kind of data.

How?

Treating the associative arrays and indexed arrays differently.

Testing Instructions

  1. Run the PHP unit tests:
    npm run test:unit:php
  2. Check that there are no failures.
  1. Add the following snippet to a PHP file, for example, to your theme's functions.php file.
  2. Check that the unwanted keys are removed from the output data.
$theme_data = array(
  'version'  => '2',
  'badKey2' => 'I am Evil!!!!',
  'settings' => array(
    'badKey2' => 'I am Evil!!!!',
    'typography' => array(
      'badKey3' => 'I am Evil!!!!',
      'fontFamilies' => array(
        array (
            'badKey4' => 'I am Evil!!!!',
            'name'       => 'Piazzolla',
            'slug'       => 'piazzolla',
            'fontFamily' => 'Piazzolla',
            'fontFace'   => [
              array(
                'badKey5' => 'I am Evil!!!!',
                'fontFamily' => 'Piazzolla',
                'fontStyle'  => 'italic',
                'fontWeight' => '400',
                'src'        => 'https://example.com/font.ttf',
              ),
              array(
                'badKey5' => 'I am Evil!!!!',
                'fontFamily' => 'Piazzolla',
                'fontStyle'  => 'italic',
                'fontWeight' => '400',
                'src'        => 'https://example.com/font.ttf',
              ),
              array(
                'badKey5' => 'I am Evil!!!!',
                'fontFamily' => 'Piazzolla',
                'fontStyle'  => 'italic',
                'fontWeight' => '400',
                'src'        => 'https://example.com/font.ttf',
              ),
            ],
        ),
        array (
            'badKey4' => 'I am Evil!!!!',
            'name'       => 'Inter',
            'slug'       => 'Inter',
            'fontFamily' => 'Inter',
            'fontFace'   => [
              array(
                'badKey5' => 'I am Evil!!!!',
                'fontFamily' => 'Inter',
                'fontStyle'  => 'italic',
                'fontWeight' => '400',
                'src'        => 'https://example.com/font.ttf',
              ),
              array(
                'badKey5' => 'I am Evil!!!!',
                'fontFamily' => 'Inter',
                'fontStyle'  => 'italic',
                'fontWeight' => '400',
                'src'        => 'https://example.com/font.ttf',
              ),
              array(
                'fontFamily' => 'Inter',
                'fontStyle'  => 'italic',
                'fontWeight' => '400',
                'src'        => 'https://example.com/font.ttf',
              ),
            ],
        )
      )
    ),
  ),
);
// Creates a new WP_Theme_JSON object with the new fonts to leverage sanitization and validation.
$theme_json = new WP_Theme_JSON_Gutenberg( $theme_data );
$data       = $theme_json->get_data();

echo('<pre>');
print_r($data);
echo('</pre>');

@github-actions GitHub Actions
Copy link

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/class-wp-theme-json-gutenberg.php
❔ phpunit/class-wp-theme-json-test.php

@matiasbenedetto matiasbenedetto added [Type] Enhancement A suggestion for improvement. [Type] Security Related to security concerns or efforts [Feature] Typography Font and typography-related issues and PRs and removed [Feature] Typography Font and typography-related issues and PRs [Type] Enhancement A suggestion for improvement. labels Nov 22, 2023
@matiasbenedetto matiasbenedetto mentioned this pull request Nov 22, 2023
Open
jffng
jffng approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@jffng jffng left a comment •
edited

Choose a reason for hiding this comment

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

I think this is a better solution than #53273 because it adds less complexity, and is more extensible in case we want to add more sanitization to keys containing indexed arrays.

I tested this according to the instructions, as well as modifying some tests in the Tests_Fonts_WPRESTFontLibraryController_InstallFonts and ensuring that the sanitization is working as expected.

We should keep an eye on any related unit tests after this is merged.

@jffng jffng enabled auto-merge (squash) November 27, 2023 15:42
@jffng jffng merged commit 41cee76 into trunk Nov 28, 2023
51 checks passed
@jffng jffng deleted the try/nested-sanitization branch November 28, 2023 16:17
@github-actions github-actions bot added this to the Gutenberg 17.2 milestone Nov 28, 2023
@jffng jffng mentioned this pull request Nov 29, 2023
Closed
derekblank pushed a commit that referenced this pull request Dec 7, 2023
@matiasbenedetto @hellofromtonya @derekblank
WP_Theme_JSON_Gutenberg: Add nested indexed array schema sanitization (…
1bfd62b 
…#56447)

* Add GB specific resolver

* changing unset function

* adding nested validation

* rename function

* Revert "Add GB specific resolver"

This reverts commit 27c0f6f.

* removing not needed check

* removing not needed check

* Remove keys from tree that are not arrays when they are defined as arrays in schema

* remove key if it is empty

* adding tests

* adding function docs

* php format

* add comment to function

---------

Co-authored-by: hellofromtonya <tonya.mork@automattic.com>
@getdave getdave mentioned this pull request Jan 18, 2024
Closed
@getdave getdave added the Needs PHP backport Needs PHP backport to Core label Jan 25, 2024
@getdave
Copy link
Contributor

getdave commented Jan 25, 2024

@matiasbenedetto Can you confirm you will backport this as part of the Fonts backport process, or shall we commit this separately?

@matiasbenedetto
Copy link
Contributor Author

matiasbenedetto commented Jan 26, 2024 •
edited

@getdave 👋 nope, I think it's convenient to sync-up this in the core repo in a different PR. This functionality is useful not just for font families related settings, but for any indexed array we want to sanitize inside WP_Theme_JSON.

@youknowriad 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 Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jffng jffng jffng approved these changes

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey

@oandregal oandregal Awaiting requested review from oandregal

Assignees
No one assigned
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Type] Security Related to security concerns or efforts
Projects
None yet
Milestone
Gutenberg 17.2
Development

Successfully merging this pull request may close these issues.

WP_Theme_JSON sanitization is not working below certain level of theme.json
5 participants
@matiasbenedetto @getdave @jffng @youknowriad @hellofromtonya