Make WordPress Core

Opened 7 months ago

Closed 4 weeks ago

Last modified 4 weeks ago

#59712 closed defect (bug) (fixed)

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

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

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.


7 months ago
#1

  • Keywords has-patch has-unit-tests added

#2 @swissspidy
3 months ago

  • Owner set to peterwilsoncc
  • Status changed from new to assigned

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

#3 @peterwilsoncc
3 months 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 months ago

  • Milestone changed from 6.5 to 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.


5 weeks ago

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


5 weeks ago

#7 @afragen
5 weeks 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 5 weeks ago by afragen (previous) (diff)

#8 @peterwilsoncc
5 weeks 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
5 weeks 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 pass the tests?

Last edited 5 weeks ago by afragen (previous) (diff)

#10 @peterwilsoncc
5 weeks 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
4 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to 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.