Skip to content

Validate font face and font family settings as JSON strings#10966

Closed
deepaklalwani97 wants to merge 2 commits intoWordPress:trunkfrom
deepaklalwani97:fix/font-rest-api-fatal
Closed

Validate font face and font family settings as JSON strings#10966
deepaklalwani97 wants to merge 2 commits intoWordPress:trunkfrom
deepaklalwani97:fix/font-rest-api-fatal

Conversation

@deepaklalwani97
Copy link

  • Ensure strict type guards for both settings params before JSON decode and return rest_invalid_param when input is not a string.
  • Add/extend REST API unit tests to cover non-string settings payloads and assert 400 error responses.

Trac ticket: https://core.trac.wordpress.org/ticket/64666

Use of AI Tools

Used Github Copilot for unit test cases which is updated and reviewed by me.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@deepaklalwani97 deepaklalwani97 marked this pull request as ready for review February 18, 2026 09:49
@github-actions
Copy link

github-actions bot commented Feb 18, 2026

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 Committers: Use this line as a base for the props when committing in SVN:

Props deepaklalwani, westonruter.

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

@github-actions
Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Comment on lines +164 to +171
// Check whether $value is a string, since it should be stringified JSON in the request.
if ( ! is_string( $value ) ) {
return new WP_Error(
'rest_invalid_param',
__( 'font_face_settings parameter must be a valid JSON string.' ),
array( 'status' => 400 )
);
}
Copy link
Member

@westonruter westonruter Feb 22, 2026

Choose a reason for hiding this comment

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

Is this not redundant? The schema already specifies that it must be a string:

Copy link
Author

@deepaklalwani97 deepaklalwani97 Feb 22, 2026

Choose a reason for hiding this comment

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

Not the check is not redundant. Even though the endpoint arg declares font_face_settings as type string, this route uses a custom validate_callback and sanitize_callback, so core does not automatically enforce type parsing for that arg before our validator runs. This explicit is_string check is a defensive guard to ensure json_decode only receives a string and to return a clean 400 error instead of risking a PHP TypeError/fatal.

Copy link
Member

@westonruter westonruter Feb 22, 2026

Choose a reason for hiding this comment

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

Alas, you're right. That's too bad. In that case, I think it would be better to look up the schema for the request and then manually enforce it:

Suggested change
// Check whether $value is a string, since it should be stringified JSON in the request.
if ( ! is_string( $value ) ) {
return new WP_Error(
'rest_invalid_param',
__( 'font_face_settings parameter must be a valid JSON string.' ),
array( 'status' => 400 )
);
}
// Enforce JSON Schema validity for field before applying custom validation logic.
$args = $this->get_endpoint_args_for_item_schema( $request->get_method() );
$validity = rest_validate_value_from_schema( $value, $args['font_family_settings'], 'font_family_settings' );
if ( is_wp_error( $validity ) ) {
return $validity;
}

You can see this is also being done similarly below:

// Check that the font face settings match the theme.json schema.
$schema = $this->get_item_schema()['properties']['font_face_settings'];
$has_valid_settings = rest_validate_value_from_schema( $settings, $schema, 'font_face_settings' );

Copy link
Author

Choose a reason for hiding this comment

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

I have updated the PR to use re-use defined schema for validation.

Comment on lines +91 to +98
if ( ! is_string( $value ) ) {
return new WP_Error(
'rest_invalid_param',
/* translators: %s: Parameter name: "font_family_settings". */
sprintf( __( '%s parameter must be a valid JSON string.' ), 'font_family_settings' ),
array( 'status' => 400 )
);
}
Copy link
Member

@westonruter westonruter Feb 22, 2026

Choose a reason for hiding this comment

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

Ditto above, the param seems to already be required to be a string:

Copy link
Author

Choose a reason for hiding this comment

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

The same applies to this endpoint as well.

Copy link
Member

Choose a reason for hiding this comment

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

See above suggestion about re-using the schema.

pento pushed a commit that referenced this pull request Feb 27, 2026
…oints for font faces and font families.

The value is expected to be a serialized JSON string, which the validation callback validates.

Developed in #10966

Follow-up to r57548.

Props deepaklalwani, westonruter.
See #59166.
Fixes #64666.


git-svn-id: https://develop.svn.wordpress.org/trunk@61765 602fd350-edb4-49c9-b593-d223f7449a82
@github-actions
Copy link

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 61765
GitHub commit: 5d90e3b

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.

@github-actions github-actions bot closed this Feb 27, 2026
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Feb 27, 2026
…oints for font faces and font families.

The value is expected to be a serialized JSON string, which the validation callback validates.

Developed in WordPress/wordpress-develop#10966

Follow-up to r57548.

Props deepaklalwani, westonruter.
See #59166.
Fixes #64666.

Built from https://develop.svn.wordpress.org/trunk@61765


git-svn-id: http://core.svn.wordpress.org/trunk@61071 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants