Make WordPress Core

Opened 6 years ago

Last modified 2 days ago

#42670 new defect (bug)

Symlinked plugin makes plugin_basename function return wrong basename

Reported by: sergiienko's profile sergiienko Owned by:
Milestone: 6.3 Priority: normal
Severity: normal Version: 4.9
Component: Plugins Keywords: has-unit-tests has-patch needs-testing needs-testing-info
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 13 months ago.
plugin_basename – Break when matching on sorted array (symlinks)
42670.2.diff (1.8 KB) - added by ironprogrammer 8 months ago.
Refresh of 42670.diff

Download all attachments as: .zip

Change History (22)

#1 @azouamauriac
16 months 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
13 months ago

plugin_basename – Break when matching on sorted array (symlinks)

#2 @brianhenryie
13 months 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 12 months ago by brianhenryie (previous) (diff)

#3 @brianhenryie
12 months ago

  • Keywords has-unit-tests has-patch added

#4 @afragen
12 months ago

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

@ironprogrammer
8 months ago

Refresh of 42670.diff

#5 @ironprogrammer
8 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
8 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.


8 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
8 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
8 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.


8 months ago

#11 @pixeldevs
8 months ago

This has been tested and works well

#12 @audrasjb
8 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.


4 months ago

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


4 months ago

#15 @costdev
4 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 4 months ago by costdev (previous) (diff)

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


3 months ago

#17 @costdev
3 months ago

  • Keywords needs-testing-info added

#18 @costdev
3 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.


2 days ago

#20 @mukesh27
2 days ago

This ticket was discussed in the recent bug scrub.

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

Note: See TracTickets for help on using tickets.