#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_packagestring: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
@
3 years ago
- Owner set to
- Status new → assigned
@peterwilsoncc Is this one on your radar for 6.5 or would you like to punt?
#3
@
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
@
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
@
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.
#8
@
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
@
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?
#10
@
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.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
https://core.trac.wordpress.org/ticket/59712#ticket