WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#29694 closed defect (bug) (fixed)

wp_not_installed() has an unbounded check for "install.php"

Reported by: nacin Owned by: joemcgill
Milestone: 4.1 Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords: good-first-bug has-patch
Focuses: Cc:

Description

This is a good-first-bug.

Steps to reproduce: Don't install WordPress. (Easy way to 'uninstall': change the table prefix.) Go to wp-admin/plugin-install.php. You'll be redirected to /wp-admin/upgrade.php. Specifically, it'll be at the root, so if you have WP installed not at the root of your domain, it'll probably fail. Also, it'll fail because WP isn't installed.

The issue is wp_not_installed() does false === strpos( $_SERVER['PHP_SELF'], 'install.php' ) which would fail for wp-admin/plugin-install.php or theme-install.php.

To fix this check we probably need to be a bit smarter. (Don't forget that PHP_SELF isn't necessarily normalized and slashes could be in different directions.) However, since we already are checking WP_INSTALLING, we can possibly excise this PHP_SELF conditional entirely.

The solution here should be paired with a quick study of the history of this code (note: it pre-dates 3.0, even though wp_not_installed() is new in 3.0, and sat probably in wp-settings.php before then) to determine why this check is there to begin with, seeing that it appears to be redundant with WP_INSTALLING. If it predates WP_INSTALLING, and the constant check was added for other reasons, that would explain it. Please cross-reference any relevant tickets and changesets you find.

Attachments (1)

29694.patch (609 bytes) - added by joemcgill 6 years ago.

Download all attachments as: .zip

Change History (7)

@joemcgill
6 years ago

#1 @joemcgill
6 years ago

  • Keywords has-patch added

tl;dr - I think it's safe to kill the check for install.php.

A brief history lesson:

  • In version 3, nacin creates wp_not_installed() from logic moved out of wp-settings.php (changeset [12732])
  • The check for strstr($_SERVER[ 'PHP_SELF']. 'install.php') dates back to v1.5, at which point there were no other *-install.php files in core.
  • Aside: Mark Jaquith commited changeset [4990] in 2.2 which replace strstr() with strpos() for performance reasons, otherwise this has been mostly unchanged forever. (related: #3920)
  • The oldest relevant changeset available in Trac [2486] shows that the check for install.php and WP_INSTALLING were originally paired as an OR instead of an AND, which would explain the redundancy.
  • Also, since define( 'WP_INSTALLING', true ); was added to install.php in 1.5.1, I'm guessing the redundancy really began to come into play at that point.
Last edited 6 years ago by SergeyBiryukov (previous) (diff)

#2 follow-up: @DrewAPicture
6 years ago

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

#3 in reply to: ↑ 2 ; follow-up: @joemcgill
6 years ago

Replying to DrewAPicture: Total trac newb question (this is my second patch, ever): anything I need to do to carry this through since I've been assigned ownership? If so, I'm happy to do so, just not sure. Thanks.

#4 in reply to: ↑ 3 ; follow-up: @DrewAPicture
6 years ago

Replying to joemcgill:

Replying to DrewAPicture: Total trac newb question (this is my second patch, ever): anything I need to do to carry this through since I've been assigned ownership? If so, I'm happy to do so, just not sure. Thanks.

Just keep on what you've been doing :) The ownership thing is just for the benefit of the good-first-bug list so that it shows as "claimed".

#5 in reply to: ↑ 4 @joemcgill
6 years ago

Replying to DrewAPicture:

Replying to joemcgill:

Replying to DrewAPicture: Total trac newb question (this is my second patch, ever): anything I need to do to carry this through since I've been assigned ownership? If so, I'm happy to do so, just not sure. Thanks.

Just keep on what you've been doing :) The ownership thing is just for the benefit of the good-first-bug list so that it shows as "claimed".

Thanks. Just didn't want to drop any balls unknowingly.

#6 @ocean90
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 30581:

Remove unbounded check for "install.php" in wp_not_installed().

The check is true for wp-admin/plugin-install.php and wp-admin/theme-install.php too. This will end in a redirect to /wp-admin/upgrade.php or a screen with a bunch of errors.
The WP_INSTALLING constant was added in WordPress 1.5.1 to wp-admin/install.php which makes the check for "install.php" redundant.

props joemcgill.
fixes #29694.

Note: See TracTickets for help on using tickets.