Make WordPress Core

Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#64574 closed defect (bug) (fixed)

Regression in `wp.sanitize.stripTags()` in WordPress 7.0-alpha

Reported by: hugod's profile hugod Owned by: westonruter's profile westonruter
Milestone: 7.0 Priority: normal
Severity: normal Version: trunk
Component: General Keywords: has-patch has-unit-tests
Focuses: javascript Cc:

Description

wp.sanitize.stripTags() doesn't return the same value when given null.

With WordPress 6.9:

console.log( 'testStripTagsWithNull', wp.sanitize.stripTags( null ) ); // ""
console.log( 'typeofResult', typeof wp.sanitize.stripTags( null ) ); // string

With WordPress 7.0-alpha:

console.log( 'testStripTagsWithNull', wp.sanitize.stripTags( null ) ); // "null"
console.log( 'typeofResult', typeof wp.sanitize.stripTags( null ) ); // string

Leading to unexpected behavior in wp.sanitize.stripTagsAndEncodeText() as well...

I think the regression has been introduced in https://github.com/WordPress/wordpress-develop/commit/3865859fd8982575204bc04e77197ea4c743ccf5.

Change History (16)

#1 @westonruter
4 months ago

  • Milestone changed from Awaiting Review to 7.0
  • Owner set to westonruter
  • Status changed from new to accepted

This ticket was mentioned in PR #10833 on WordPress/wordpress-develop by @westonruter.


4 months ago
#2

  • Keywords has-patch has-unit-tests added


Ensure that wp.sanitize.stripTags() returns an empty string when the input is null or undefined, preventing it from being converted to the string "null" by DOMParser.

Added regression tests.

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

#3 @westonruter
4 months ago

  • Version changed from trunk to 6.9

@hugod Thanks for reporting this.

This indeed was introduced in [60907] to fix #48054.

I have an initial PR to fix the issue here available for testing and review: https://github.com/WordPress/wordpress-develop/pull/10833

#4 @westonruter
4 months ago

  • Version changed from 6.9 to trunk

Actually, the regression wasn't introduced in [60907] but rather in [61347] to fix #64274.

@mukesh27 commented on PR #10833:


3 months ago
#5

@westonruter Can you share testing steps please?

@westonruter commented on PR #10833:


3 months ago
#6

@mukeshpanchal27 the ticket description has the reproduction info. I guess go to a screen where that script is enqueued, or enqueued the script yourself, and then run the function in the console.

#7 @westonruter
3 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 61578:

General: Preserve back-compat for wp.sanitize.stripTags() to return empty string when passed null/undefined.

This also bumps the esversion to 11 in tests/qunit/.jshintrc to match the root .jshintrc, following [61544] for #64562.

Follow-up to [61347], [60907].

Props westonruter, jonsurrell, mukesh27, hugod.
See #64274.
Fixes #64574.

#9 @westonruter
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#10 @westonruter
3 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 61585:

General: Further preserve back-compat for wp.sanitize.stripTags() to return empty string when falsy value supplied.

Developed in https://github.com/WordPress/wordpress-develop/pull/10856

Follow-up to [61578], [61347], [60907].

Props jonsurrell, hugod, westonruter.
See #64274.
Fixes #64574.

This ticket was mentioned in PR #10994 on WordPress/wordpress-develop by @westonruter.


3 months ago
#11

See https://github.com/WordPress/wordpress-develop/pull/10833#discussion_r2833992103:

I’m late to the game, but do we expect any other type errors? I suppose we expect people to send things that are strings nominally, but may be missing.

However, if we make a _positive assertion_ we wouldn’t have to chase this later.

if ( 'string' !== typeof $text ) {
        return '';
}

It turns out that passing a non-string in 6.9 would cause an error due to the replace() method not being available on it (probably). So the previous change in r61578 which added support for sanitizing numbers actually would have previously caused an error. So we can indeed just short-circuit when a non-string is passed.

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

## Use of AI Tools

None

@westonruter commented on PR #10994:


3 months ago
#12

@Hug0-Drelon How does this work for you?

@hugod commented on PR #10994:


3 months ago
#13

@westonruter LGTM 👍

@jonsurrell commented on PR #10994:


3 months ago
#14

I've looked at 6.8.3, and the regression seems to be that any falsy value is treated as the string `''`:

let _text = text || '';

As such, numbers (aside from 0) would error because of the missing .replace() method and don't need to be handled here.

#15 @westonruter
3 months ago

In 61783:

General: Update wp.sanitize.stripTags() to return empty string when not passed a string.

Developed in https://github.com/WordPress/wordpress-develop/pull/10994

Follow-up to r61585, r61578.

Props westonruter, jonsurrell, dmsnell, hugod.
See #64574, #64274.

@westonruter commented on PR #10994:


3 months ago
#16

Committed in r61783 (7e46834)

Note: See TracTickets for help on using tickets.