Make WordPress Core

Opened 7 years ago

Last modified 3 weeks ago

#42670 assigned defect (bug)

Symlinked plugin makes plugin_basename function return wrong basename

Reported by: sergiienko's profile sergiienko Owned by: brianhenryie's profile brianhenryie
Milestone: 6.7 Priority: normal
Severity: normal Version: 4.9
Component: Plugins Keywords: has-unit-tests needs-testing has-patch has-testing-info dev-feedback
Focuses: Cc:

Description

Symlinked plugin makes plugin_basename function return wrong basename for plugins which goes after the symlinked plugin. If symlinked plugin name is substring of WP root directory name.

For instance, plugin name is feedback-plugin and WP root directory is feedback-plugin-wp.

Cause of such a behavior is condition:

// wp-includes/plugin.php plugin_basename function (line 658).
if ( strpos( $file, $realdir ) === 0 ) {
    $file = $dir . substr( $file, strlen( $realdir ) );
}

Solution:

$pattern = '/^' . str_replace('/', '\/', $realdir) . '\//';
if ( preg_match( $pattern, $file ) ) {
    $file = $dir . substr( $file, strlen( $realdir ) );
}

Attachments (2)

42670.diff (2.1 KB) - added by brianhenryie 2 years ago.
plugin_basename – Break when matching on sorted array (symlinks)
42670.2.diff (1.8 KB) - added by ironprogrammer 22 months ago.
Refresh of 42670.diff

Download all attachments as: .zip

Change History (46)

#1 @azouamauriac
2 years ago

  • Keywords reporter-feedback added

Hello there, welcome to the trac, thanks for the report, please can you provide steps to reproduce the issue?

The code was introduced here: [27158]

thanks.

@brianhenryie
2 years ago

plugin_basename – Break when matching on sorted array (symlinks)

#2 @brianhenryie
2 years ago

This issue is a continuation of [28441] where [@ocean90] introduced arsort() in patch [37983] to address the same problem.

The reasoning behind using arsort() is sound, but the patch failed to break after matching and a scenario exists where the wrong basename can still be returned.

Given a project structure:

/Users/brianhenry/Sites/my-plugin
/Users/brianhenry/Sites/my-plugin/my-plugin.php (An empty plugin, only headers)
/Users/brianhenry/Sites/my-plugin/wordpress (WordPress installed here)
/Users/brianhenry/Sites/my-plugin/wordpress/wp-content (Symlink to /Users/brianhenry/Sites/my-plugin/wordpress/wp-content)
/Users/brianhenry/Sites/my-plugin/wp-content
/Users/brianhenry/Sites/my-plugin/wp-content/plugins/my-plugin (Symlink to /Users/brianhenry/Sites/my-plugin/)
/Users/brianhenry/Sites/my-plugin/wp-content/plugins/woocommerce

Looking inside plugin_basename(), after arsort() $wp_plugins_path will look like:

array (
  '/Users/brianhenry/Sites/my-plugin/wordpress/wp-content/plugins/woocommerce' => '/Users/brianhenry/Sites/my-plugin/wp-content/plugins/woocommerce',
  '/Users/brianhenry/Sites/my-plugin/wordpress/wp-content/plugins/my-plugin' => '/Users/brianhenry/Sites/my-plugin',
)

Running plugin_basename( __FILE__ ) for WooCommerce means plugin_basename( '/Users/brianhenry/Sites/my-plugin/wp-content/plugins/woocommerce/woocommerce.php' ).

The operation if ( strpos( $file, $realdir ) === 0 ) (does $file begin with $realdir?) evaluates to true for both values in the array.

Using arsort() is certainly key to solving this, but the for loop must break once it matches. i.e. use the most specific match (the longest), any subsequent matches will be less specific.

This patch adds only break;.

Last edited 2 years ago by brianhenryie (previous) (diff)

#3 @brianhenryie
2 years ago

  • Keywords has-unit-tests has-patch added

#4 @afragen
2 years ago

  • Keywords needs-testing added; reporter-feedback removed
  • Milestone changed from Awaiting Review to 6.1

@ironprogrammer
22 months ago

Refresh of 42670.diff

#5 @ironprogrammer
22 months ago

In 42670.2.diff, refreshes patch from @brianhenryie:

  • Adjust spacing for WPCS standards in plugin.php.
  • Use assertSame() in unit test.
  • Add assertion failure message.
  • Update test for readability.

Thank you, @brianhenryie!

#6 @desrosj
22 months ago

  • Keywords changes-requested added

@ironprogrammer could you open a PR with 42670.2.diff just to confirm all of the tests complete successfully?

Also, could we replace using WooCommerce with one maintained by the Core team? Maybe the WordPress Importer?

This ticket was mentioned in PR #3412 on WordPress/wordpress-develop by ironprogrammer.


22 months ago
#7

In this PR:

  • Adjust spacing for WPCS standards in plugin.php.
  • Use assertSame() in unit test.
  • Add assertion failure message.
  • Update test for readability.
  • Change test reference to wordpress-importer.

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

#8 @ironprogrammer
22 months ago

  • Keywords changes-requested removed

In response to @desrosj:

Opened PR #3412 per comment:6 👍🏻 Update also resolves test's dirname() $levels compatibility issue with PHP 5.6.

This PR supersedes 42670.2.diff.

#9 @afragen
22 months ago

@ironprogrammer I added a question on the PR re: global.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


22 months ago

#11 @pixeldevs
22 months ago

This has been tested and works well

#12 @audrasjb
21 months ago

  • Milestone changed from 6.1 to 6.2

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next one or two hours, please feel free to move this ticket back to milestone 6.1.

This ticket was mentioned in Slack in #core by costdev. View the logs.


17 months ago

This ticket was mentioned in Slack in #core-test by costdev. View the logs.


17 months ago

#15 @costdev
17 months ago

This ticket was discussed during the recent bug scrub and still needs test reports. I have dropped this ticket into the Test Team channel to try to get some eyes on this ticket.

@ironprogrammer Would it be possible to get some testing instructions for the patch?

Last edited 17 months ago by costdev (previous) (diff)

This ticket was mentioned in Slack in #core by costdev. View the logs.


17 months ago

#17 @costdev
17 months ago

  • Keywords needs-testing-info added

#18 @costdev
17 months ago

  • Milestone changed from 6.2 to 6.3

This ticket was discussed in the bug scrub and it was agreed to move this ticket to 6.3.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


14 months ago

#20 @mukesh27
14 months ago

This ticket was discussed in the recent bug scrub.

It looks like we are waiting some testing info from @ironprogrammer.

#21 @brianhenryie
13 months ago

Testing Instructions

https://github.com/BrianHenryIE/a-wp-trac-42670

The test itself is straightforward but the test environment may be specific: I run Apache on MacOS.

Last edited 13 months ago by brianhenryie (previous) (diff)

This ticket was mentioned in Slack in #core-test by brianhenryie. View the logs.


13 months ago

This ticket was mentioned in Slack in #core-test by ironprogrammer. View the logs.


13 months ago

#24 @brianhenryie
13 months ago

  • Keywords needs-testing-info removed

This ticket was mentioned in Slack in #core by oglekler. View the logs.


12 months ago

This ticket was mentioned in Slack in #core by chaion07. View the logs.


12 months ago

#27 @chaion07
12 months ago

Thanks @sergiienko for reporting this. We reviewed this ticket today during a recent bug-scrub session and based on the feedback we are updating the milestone to 6.4. Cheers!

Props to @costdev, @oglekler and @mukesh27

#28 @chaion07
12 months ago

  • Milestone changed from 6.3 to 6.4

#29 @brianhenryie
12 months ago

After I posted the repo above to reproduce the issue and verify the patch, @ironprogrammer and I discovered another related bug that should be fixed at the same time: my patch was written for an environment where there is a symlink inside a symlink (/wordpress/wp-content -> /wp-content, and /wp-content/plugins/example-plugin -> /). Where there is only one symlink (wordpress/wp-content/plugins -> .) the issue remains.

I'm posting notes from the Slack thread here:


So... [wp_register_plugin_realpath()](https://github.com/WordPress/WordPress/blob/9e0f2faa28f1aa04a69e0b4eaa410a38adcd2e1e/wp-includes/plugin.php#L819-L821) doesn't record the plugin realpath to $wp_plugin_paths if its calculated realpath() isn't any different to the WP_PLUGIN_DIR subdirectory path.

Then when $wp_plugin_paths is looped over in plugin_basename(), where the "fix" is, the only entry is the only symlinked plugin, i.e. a-wp-trac-42670 in the project root, which then matches for every plugin queried in plugin_basename().

The bug can be fixed by removing the condition on whether the plugins' paths are recorded.

Ideally $wp_plugin_paths would only be populated when there is one or more symlinked plugins, but we can't know this until looking at each one.

Something like this could be used to empty the array afterwards in times when it's not needed:

<?php
add_action( 'plugins_loaded', function() {
        global $wp_plugin_paths;

        foreach ( $wp_plugin_paths as $dir => $realdir ) {
                if( $dir !== $realdir ) {
                        return;
                }
        }

        $wp_plugin_paths = array();     
}, 0);

Reflecting on that, the minimum addition to the patch should be:

In wp-includes/plugin.php:819 change:

<?php
if ( $plugin_path !== $plugin_realpath ) {
  $wp_plugin_paths[ $plugin_path ] = $plugin_realpath;
}

to:

<?php
$wp_plugin_paths[ $plugin_path ] = $plugin_realpath;

The plugins_loaded code above might be best omitted for clarity. The performance impact here is how many times the sorted global $wp_plugin_paths array, whose length will be the number of plugins installed, is traversed by plugin_basename() (an admittedly popular function).

This ticket was mentioned in Slack in #core by nicolefurlan. View the logs.


10 months ago

#31 @nicolefurlan
10 months ago

  • Keywords needs-refresh added; has-patch removed
  • Owner set to brianhenryie
  • Status changed from new to assigned

@brianhenryie is taking ownership of this ticket. He will refresh the patch, update testing instructions, and get it ready for review again. Thank you Brian!

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


10 months ago
#32

  • Keywords has-patch added; needs-refresh removed

Continuing from https://github.com/WordPress/wordpress-develop/pull/3412 (@ironprogrammer)

The latest change removes the conditional if ( $plugin_path !== $plugin_realpath ) { from inside wp_register_plugin_realpath() so all plugin paths are registered.

The existing test in this PR, test_should_return_correct_basename_for_plugin_when_wp_plugins_dir_is_subdir_of_symlinked_plugin() assumed $wp_plugin_paths would always contain the plugin path, which was not correct, due to the conditional now removed.

Test instructions with wp-env and pictures: https://github.com/BrianHenryIE/a-wp-trac-42670

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

#33 @brianhenryie
10 months ago

  • Keywords has-testing-info added

wp-env test instructions for updated patch: https://github.com/BrianHenryIE/a-wp-trac-42670

Discussion:

This patch affects plugin_basename() and wp_register_plugin_realpath(). Previously, only plugins whose path and realpath differed were recorded in the global $wp_plugin_paths array, meaning a site which does not use symlinks would have an empty array and the foreach ( $wp_plugin_paths ... loop in plugin_basename() would be passed over.

Now, every time plugin_basename() is called, that loop will run on the now-populated $wp_plugin_paths array.

Although plugin_basename() is widely used, my gut says that will be insignificant to performance. I would like someone with better intuition to give their opinion.

This ticket was mentioned in Slack in #core-performance by brianhenryie. View the logs.


10 months ago

This ticket was mentioned in Slack in #core-test by brianhenryie. View the logs.


9 months ago

This ticket was mentioned in Slack in #core by oglekler. View the logs.


9 months ago

#37 @oglekler
9 months ago

  • Milestone changed from 6.4 to 6.5

We have RC1 tomorrow and have no time for testing, so, I am moving this to the next milestone. Let's try to make it in the alpha :)

#38 @swissspidy
5 months ago

Is this on anyone's radar still for 6.5? Looks like a punt candidate given the lack of activity.

#39 @swissspidy
5 months ago

  • Milestone changed from 6.5 to 6.6

This ticket was mentioned in Slack in #core by oglekler. View the logs.


5 weeks ago

#41 @oglekler
5 weeks ago

This ticket has testing instructions in the #comment:33

This ticket was mentioned in Slack in #core by nhrrob. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #core-test by nhrrob. View the logs.


4 weeks ago

#44 @oglekler
3 weeks ago

  • Keywords dev-feedback added
  • Milestone changed from 6.6 to 6.7

This ticket needs developers to test, and it looks like because of the testing complication we have no volunteers to do this. I am moving this ticket to the next milestone and adding dev-feedback in hope that we will get more attention from developers.

Note: See TracTickets for help on using tickets.