WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 4 months ago

#53946 new enhancement

Improve return types for sanitizing/escaping functions

Reported by: malthert Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Security Keywords:
Focuses: docs Cc:

Description

The return type of functions where param is string|array, specificall escaping and sanitizing functions, should be set more specific (= that the function returns the same type the passed $arg has).

This means changing:

<?php
     * @param string|array $value String or array of data to unslash.
     * @return string|array Unslashed $value.
     */
    function wp_unslash($value)

to this:

     * @param string|array $value String or array of data to unslash.
     * @return ($value is array ? array : string) Unslashed $value.
     */
    function wp_unslash($value)

This ensures that static analysis tools correctly infer the type, as well as make it clear to developers that the variable type is not changed by this function.

This is filed as bug, since it is a (more and more) deprecated way of declaring phpdocs, thus should be updated.

Change History (6)

#1 in reply to: ↑ description @malthert
4 months ago

Specifically, this is for:
public function _escape
public function escape
function esc_sql
function wp_slash
function wp_unslash

#2 @SergeyBiryukov
4 months ago

  • Component changed from General to Security
  • Focuses docs added

#3 @johnbillion
4 months ago

  • Keywords reporter-feedback added
  • Type changed from defect (bug) to enhancement
  • Version trunk deleted

Thanks for the report @malthert . Can you point us to the documentation for this syntax please?

#4 @malthert
4 months ago

The documentation for phpdocs is extremely fragmented, so I am not really sure where this is from.

I just found https://github.com/vimeo/psalm/issues/4629#issuecomment-730570590 and this seems to work for psalm and phpstan (besides the fact, that this is easily clearer than what is there now)

#5 follow-up: @johnbillion
4 months ago

  • Keywords reporter-feedback removed

Thanks for the link!

This syntax appears to be specific to Psalm and it isn't supported by PHPStan (although it's on the roadmap), nor is it supported by phpDocumentor, so I'm inclined to say that as much as it would be great to have machine-readable conditional return types I don't think we should introduce a syntax which breaks existing tooling that isn't Psalm.

We could use the Psalm-specific @psalm-return tag but that would imply some level of support for Psalm in core which there isn't. There's some discussion on whether we should introduce PHPStan-specific syntax and configuration in #52217 and the same would apply to Psalm.

I think this is a wontfix, unfortunately, but I would welcome a patch which improves the descriptions of these functions in order to better explain the possible return types.

#6 in reply to: ↑ 5 @SergeyBiryukov
4 months ago

Replying to johnbillion:

I think this is a wontfix, unfortunately, but I would welcome a patch which improves the descriptions of these functions in order to better explain the possible return types.

I've found some similar instances mentioning "same type":

  • sanitize_bookmark():
     * @param stdClass|array $bookmark Bookmark row.
     * @param string         $context  Optional. How to filter the fields. Default 'display'.
     * @return stdClass|array Same type as $bookmark but with fields sanitized.
     */
    function sanitize_bookmark( $bookmark, $context = 'display' ) {
    
  • sanitize_category():
     * @param object|array $category Category data.
     * @param string       $context  Optional. Default 'display'.
     * @return object|array Same type as $category with sanitized data for safe use.
     */
    function sanitize_category( $category, $context = 'display' ) {
    
  • sanitize_category_field()
     * @param string $field   Category key to sanitize.
     * @param mixed  $value   Category value to sanitize.
     * @param int    $cat_id  Category ID.
     * @param string $context What filter to use, 'raw', 'display', etc.
     * @return mixed Same type as $value after $value has been sanitized.
     */
    function sanitize_category_field( $field, $value, $cat_id, $context ) {
    
  • sanitize_user_object():
     * @param object|array $user    The user object or array.
     * @param string       $context Optional. How to sanitize user fields. Default 'display'.
     * @return object|array The now sanitized user object or array (will be the same type as $user).
     */
    function sanitize_user_object($user, $context = 'display') {
    
  • sanitize_post():
     * @param object|WP_Post|array $post    The post object or array
     * @param string               $context Optional. How to sanitize post fields.
     *                                      Accepts 'raw', 'edit', 'db', 'display',
     *                                      'attribute', or 'js'. Default 'display'.
     * @return object|WP_Post|array The now sanitized post object or array (will be the
     *                              same type as `$post`).
     */
    function sanitize_post( $post, $context = 'display' ) {
    
  • wpdb::_escape()
     * @param string|array $data Data to escape.
     * @return string|array Escaped data, in the same type as supplied.
     */
    public function _escape( $data ) {
    

So I think something like this would work here as well:

 * @param string|array $data Unescaped data.
 * @return string|array Escaped data, in the same type as supplied.
 */
function esc_sql( $data ) {

...
 * @param string|array $value String or array of data to slash.
 * @return string|array Slashed $value, in the same type as supplied.
 */
function wp_slash( $value ) {

...
* @param string|array $value String or array of data to unslash.
* @return string|array Unslashed $value, in the same type as supplied.
*/
function wp_unslash( $value )
Note: See TracTickets for help on using tickets.