Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#59712 closed defect (bug) (fixed)

`WP_Ugrader` doesn't check source and destination variable types, is missing a string.

Reported by: peterwilsoncc Owned by: peterwilsoncc
Priority: normal Milestone: 6.6
Component: Upgrade/Install Version:
Severity: normal Keywords: has-patch has-unit-tests needs-testing
Cc: Focuses:

Description

As discovered by @costdev, @jipmoors and @karlijnbok while working on #54245 the WP_Ugrader class:

  • fails to accurately check the source and destination directories are valid strings
  • is missing the no_package string: Package not available.

These fixes were initially included in ​PR #3044 but went uncommitted as the tests were committed during the release candidate phase of the release cycle.

Change History (12)

This ticket was mentioned in ​PR #5558 on ​WordPress/wordpress-develop by ​@peterwilsoncc.


3 years ago
#1

  • Keywords has-patch has-unit-tests added

#2 @swissspidy
3 years ago

  • Owner set to peterwilsoncc
  • Status new → assigned

@peterwilsoncc Is this one on your radar for 6.5 or would you like to punt?

#3 @peterwilsoncc
3 years ago

  • Keywords needs-testing added

@swissspidy I think it would be good to get some manual testing done first. Are you happy to keep it on the milestone for a few days and punt on Wednesday or Thursday if nothing happens?

#4 @swissspidy
3 years ago

  • Milestone 6.5 → 6.6

I'm punting for now based on the above. Feel free to move back if there's been some testing in the meantime.

This ticket was mentioned in ​Slack in #core-upgrade-install by afragen. ​View the logs.


3 years ago

This ticket was mentioned in ​Slack in #core-upgrade-install by peterwilsoncc. ​View the logs.


3 years ago

#7 @afragen
3 years ago

In testing the PR does function correctly but I found a possible flaw. If $args['source'] is a string and has leading or trailing spaces it passes the PR check, however, it fails the following line 566. I don’t even know if this edge case is possible. If not then the PR tests out fine.

$source = trailingslashit( $args['source'] ) . trailingslashit( $source_files[0] );

I also ran tests separately and all passing.

Last edited 3 years ago by afragen (previous) (diff)

#8 @peterwilsoncc
3 years ago

@afragen @costdev Do you think the check should be this instead?

<?php
if (
        ( ! is_string( $source ) || $source !== trim( $source ) ) ||
        ( ! is_string( $destination ) || $destination !== trim( $destination ) )
) {

An alternative would be to reassign the values with their trimmed versions and check for an empty string.

#9 @afragen
3 years ago

@peterwilsoncc that looks like it should cover the edge case above and upon a closer look does seem that it should catch empty strings. Does it work with the tests?

Version 1, edited 3 years ago by afragen (previous) (next) (diff)

#10 @peterwilsoncc
3 years ago

Good catch, Andy, I've used your suggestion and pushed it with some additional test cases.

It's possibly a little overkill but if WordPress is going to test for invalid strings, it should at least do it completely.

#11 @peterwilsoncc
2 years ago

  • Resolution → fixed
  • Status assigned → closed

In 58022:

Upgrade/Install: Validate source & destination values in WP_Ugrader.

Adds a missing string and some additional validation of paths in the upgrader class.

Follow up to [56992].

Props costdev, jipmoors, karlijnbok, swissspidy, afragen, mukesh27.
Fixes #59712.

Note: See TracTickets for help on using tickets.