Skip to content

Font Library: sanitize font collection data - #58636

Merged
youknowriad merged 9 commits into
trunkfrom
add/sanitize-font-collection-data
Feb 5, 2024
Merged

Font Library: sanitize font collection data#58636
youknowriad merged 9 commits into
trunkfrom
add/sanitize-font-collection-data

Conversation

@matiasbenedetto

@matiasbenedetto matiasbenedetto commented Feb 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

What?

Font Library: sanitize font collection data.

  • Removes all the potential unwanted properties.
  • Sanitizes all the values.
  • Adds unit test case to ensure that data returned by get_data is always sanitized.

Why?

To return just the data that's safe to return.

How?

  • Adding a data schema in the WP_Font_Collection class.
  • Sanitizing the data using the sanitizing util from WP_Font_Utils

Testing Instructions

Run this PHP snippet featuring a font collection with risky data:

function register_collections() {
    $ubuntu = array(
        'font_family_settings' => array (
            'name'       => 'Ubuntu<script>alert("xss")</script>',
            'fontFamily' => 'Ubuntu, sans-serif<script>alert("xss")</script>',
            'slug'       => 'ubuntu<script>alert("xss")</script>',
        ),
        'categories' => array(
            'sans-serif',
        ),
    );
    
    $verdana = array(
        'font_family_settings' => array (
            'name'       => 'Verdana<script>alert("xss")</script>',
            'fontFamily' => 'Verdana, sans-serif<script>alert("xss")</script>',
            'slug'       => 'verdana<script>alert("xss")</script>',
        ),
        'categories' => array(
            'sans-serif',
        ),
    );
    
    $font_families = array ( $ubuntu, $verdana );
    
    $categories = array(
        array (
            'name' => 'Sans Serif<script>alert("xss")</script>',
            'slug' => 'sans-serif<script>alert("xss")</script>',
        ),
    );
    
    $collection_with_xss = array(
        'name'          => 'PHP Custom Collection',
        'description'   => __( 'Custom fonts collection' ),
        'font_families' => $font_families,
        'categories'    => $categories
    );
    
    wp_register_font_collection( 'collection-with-xss', $collection_with_xss );
}

add_action( 'rest_api_init', 'register_collections' );

Request that font collection using the API:

/wp-json/wp/v2/font-collections/collection-with-xss

the response should be:

{
    "slug": "collection-with-xss",
    "name": "PHP Custom Collection",
    "description": "Custom fonts collection",
    "font_families": [
        {
            "font_family_settings": {
                "name": "Ubuntu",
                "fontFamily": "Ubuntu, sans-serif",
                "slug": "ubuntualertxss"
            },
            "categories": [
                "sans-serif"
            ]
        },
        {
            "font_family_settings": {
                "name": "Verdana",
                "fontFamily": "Verdana, sans-serif",
                "slug": "verdanaalertxss"
            },
            "categories": [
                "sans-serif"
            ]
        }
    ],
    "categories": [
        {
            "name": "Sans Serif",
            "slug": "sans-serifalertxss"
        }
    ],
    "_links": {
        "self": [
            {
                "href": "http://localhost/wp1/wp-json/wp/v2/font-collections/collection-with-xss"
            }
        ],
        "collection": [
            {
                "href": "http://localhost/wp1/wp-json/wp/v2/font-collections"
            }
        ]
    }
}

@github-actions

github-actions Bot commented Feb 2, 2024 •
edited
Loading

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core SVN

If you're a Core Committer, use this list when committing to wordpress-develop in SVN:

Props: mmaattiiaass, grantmkin, youknowriad.

GitHub Merge commits

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org>
Co-authored-by: creativecoder <grantmkin@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@matiasbenedetto matiasbenedetto added [Type] Security Related to security concerns or efforts [Feature] Font Library labels Feb 2, 2024
@github-actions

github-actions Bot commented Feb 2, 2024 •
edited
Loading

Copy link
Copy Markdown

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/compat/wordpress-6.5/fonts/class-wp-font-collection.php
❔ lib/compat/wordpress-6.5/fonts/class-wp-font-utils.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/__construct.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/getData.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getFontCollection.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/registerFontCollection.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/unregisterFontCollection.php

creativecoder
creativecoder reviewed Feb 2, 2024
View reviewed changes
Comment thread lib/compat/wordpress-6.5/fonts/class-wp-font-collection.php Outdated
Comment thread lib/compat/wordpress-6.5/fonts/class-wp-font-collection.php Outdated
@creativecoder
creativecoder force-pushed the add/sanitize-font-collection-data branch from 95389bf to a581183 Compare February 3, 2024 01:26
@creativecoder
Updates how collection data is sanitized and validated
f307972 
- Sanitize and validate as early as possible, so that appropriate notices are logged right away
- Use static method for sanitization schema so that we can use a closure rather than a public method for src sanitization
- Adds a check for WP_Font_Utils::sanitize_from_schema so that class callables like array( $this, 'sanitization_method'	) can be used
- Updates method name to indicate that sanitization is done first, as this might remove invalid data and affect the validation result
@creativecoder
creativecoder force-pushed the add/sanitize-font-collection-data branch from a581183 to f307972 Compare February 3, 2024 01:27
@creativecoder

creativecoder commented Feb 3, 2024

Copy link
Copy Markdown
Contributor

@matiasbenedetto I made an attempt to streamline this and pushed a commit to this branch:

  • Sanitize and validate as early as possible, so that appropriate notices are logged right away.
  • Use static method for sanitization schema rather than a class constant so that we can use a closure for src sanitization rather than needing to add a new public method to the class.
  • Adds a check within WP_Font_Utils::sanitize_from_schema so that class callables like array( $this, 'sanitization_method' ) can also be used
  • Updates method name to sanitize_and_validate_data to indicate that sanitization is done first, as it might remove invalid data and affect the validation result.

Feel free to revert if you don't agree with the changes. Otherwise I think this now looks ready to go.

creativecoder
creativecoder reviewed Feb 3, 2024
View reviewed changes
Comment thread lib/compat/wordpress-6.5/fonts/class-wp-font-utils.php

$is_value_array = is_array( $value );
$is_schema_array = is_array( $schema[ $key ] );
$is_schema_array = is_array( $schema[ $key ] ) && ! is_callable( $schema[ $key ] );

@creativecoder creativecoder Feb 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Adding this check allows using a class instance method, like array( $this, 'sanitization_method' ).

@creativecoder creativecoder mentioned this pull request Feb 5, 2024
Closed
25 tasks done
@youknowriad
youknowriad merged commit b942f10 into trunk Feb 5, 2024
@youknowriad
youknowriad deleted the add/sanitize-font-collection-data branch February 5, 2024 08:39
@github-actions github-actions Bot added this to the Gutenberg 17.7 milestone Feb 5, 2024
@creativecoder creativecoder mentioned this pull request Feb 6, 2024
Closed
@sirreal sirreal mentioned this pull request Mar 27, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@creativecoder creativecoder creativecoder left review comments
@youknowriad youknowriad youknowriad approved these changes
@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey spacedmonkey is a code owner
@pbking pbking Awaiting requested review from pbking

Assignees

No one assigned

Labels

[Feature] Font Library [Type] Security Related to security concerns or efforts

Projects

None yet

Milestone

Gutenberg 17.7

Development

Successfully merging this pull request may close these issues.

3 participants

@matiasbenedetto @creativecoder @youknowriad