Make WordPress Core

Opened 2 months ago

Closed 2 months ago

Last modified 2 weeks ago

#60758 closed defect (bug) (fixed)

Interactivity API: Bind directives may access uninitialized string offset

Reported by: jonsurrell's profile jonsurrell Owned by: swissspidy's profile swissspidy
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.5
Component: Interactivity API Keywords: has-patch has-unit-tests commit fixed-major dev-reviewed
Focuses: Cc:

Description

When using bind directives with short attribute names (less than 5 characters), the following warning is produced:

Warning: Uninitialized string offset 4 in /var/www/html/wp-includes/interactivity-api/class-wp-interactivity-api.php on line 579

This should be fixed to prevent the warning.

Change History (21)

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


2 months ago
#1

  • Keywords has-patch added

Fix the following warning when using an interactivity bind directive with a short attribute name, e.g. wp-data-bind--id.

Warning: Uninitialized string offset 4 in /var/www/html/wp-includes/interactivity-api/class-wp-interactivity-api.php on line 579

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

@jonsurrell commented on PR #6257:


2 months ago
#2

👋 @c4rl0sbr4v0 @DAreRodz for review.

#3 @swissspidy
2 months ago

  • Milestone changed from Awaiting Review to 6.5
  • Version set to trunk

@swissspidy commented on PR #6257:


2 months ago
#4

Possible to add tests for this?

#5 @swissspidy
2 months ago

  • Component changed from General to Editor
  • Keywords needs-unit-tests added

@jonsurrell commented on PR #6257:


2 months ago
#6

I tried adding tests and would like to, but I couldn't get them to fail on the warning, that may be a configuration option or my lack of knowledge, but I couldn't find any way of saying "warnings on _this test_ should cause the test to fail."

There is actually a test that has a short attribute and should hit this code path, but it doesn't fail the test suite or seem to generate any warning:

https://github.com/WordPress/wordpress-develop/blob/a6783cfbee4f72b38d67744c2a456b89dce626ab/tests/phpunit/tests/interactivity-api/wpInteractivityAPI-wp-bind.php#L368-L381

@gziolo commented on PR #6257:


2 months ago
#7

I can see the error when running tests. I might be valuable to add another test that handles the boolean value:

https://github.com/WordPress/wordpress-develop/assets/699132/c12bd5a0-cf88-480a-8795-d64d1a061c7a

The same check needs to be added in another line which only will trigger when passing true:

if ( is_bool( $result ) && strlen( $bound_attribute ) > 5 && '-' === $bound_attribute[4] ) {

@jonsurrell commented on PR #6257:


2 months ago
#8

Good catch! I pushed a change to apply the same fix to that warning.

@gziolo commented on PR #6257:


2 months ago
#9

You also need a test like the one I shared to trigger the warning in the second updated line. The value needs to be true.

@jonsurrell commented on PR #6257:


2 months ago
#10

You also need a test like the one I shared to trigger the warning in the second updated line. The value needs to be true.

I've pushed a test for this, but I don't see the warnings when running the tests 😕

@jonsurrell commented on PR #6257:


2 months ago
#11

You also need a test like the one I shared to trigger the warning in the second updated line. The value needs to be true.

I've pushed a test for this, but I don't see the warnings when running the tests 😕

@jonsurrell commented on PR #6257:


2 months ago
#12

I pushed a test that does error if I revert the changes.

@jonsurrell commented on PR #6257:


2 months ago
#13

I'll address the test failures.

@jonsurrell commented on PR #6257:


2 months ago
#14

I fixed the tests and updated them to use assertSame. We're comparing values of types we expect to match, mostly strings, so assertSame is the appropriate assertion.

#15 @swissspidy
2 months ago

  • Keywords has-unit-tests commit dev-feedback added; needs-unit-tests removed

#16 @swissspidy
2 months ago

  • Owner set to swissspidy
  • Resolution set to fixed
  • Status changed from new to closed

In 57835:

Interactivity API: Prevent warning when using a bind directive with a short attribute name.

Adds new tests and improves existing ones by using assertSame to do type comparison as well.

Props jonsurrell, cbravobernal, swissspidy, gziolo.
Fixes #60758.

#18 @swissspidy
2 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#19 @gziolo
2 months ago

  • Keywords dev-reviewed added; dev-feedback removed

#20 @swissspidy
2 months ago

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

In 57837:

Interactivity API: Prevent warning when using a bind directive with a short attribute name.

Adds new tests and improves existing ones by using assertSame to do type comparison as well.

Reviewed by gziolo.
Merges [57835] to the to the 6.5 branch.

Props jonsurrell, cbravobernal, swissspidy, gziolo.
Fixes #60758.

#21 @jonsurrell
2 weeks ago

  • Component changed from Editor to Interactivity API
Note: See TracTickets for help on using tickets.