WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 6 months ago

Last modified 5 months ago

#25317 closed defect (bug) (fixed)

Wrong guess in wp_guess_url() for symlinked install, or unit tests

Reported by: bpetty Owned by:
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.7
Component: Upgrade/Install Keywords: needs-unit-tests close
Focuses: Cc:

Description

The changes made in r25396 add additional checks, but could use a couple more fixes.

  1. In the case of symlinked installations (checkout of develop.svn, and symlink public_html to 'src' for example), this fails to properly detect the URL, and ends up appending a full path disclosure to the siteurl option.
  2. In the case of unit tests, this also fails to properly strip the absolute file path off (since SCRIPT_FILENAME points to tests/phpunit/includes/install.php instead), causing most canonical unit tests to fail.

For (A), a confirmed solution is using realpath() on SCRIPT_FILENAME first.

In the case of (B), we should probably just set WP_SITEURL in bootstrap.php most likely - it would help keep wp_guess_url() simple and specific to real installations.

If no-one else jumps on it, I can easily take care of (B), but still need a patch on (A).

Attachments (2)

25317.patch (2.1 KB) - added by SergeyBiryukov 7 months ago.
25317.2.patch (2.0 KB) - added by SergeyBiryukov 7 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 bpetty7 months ago

Related: #16884, #24173, and #24480

SergeyBiryukov7 months ago

comment:2 SergeyBiryukov7 months ago

  • Keywords has-patch added; needs-patch removed
  • Priority changed from normal to highest omg bbq
  • Severity changed from normal to blocker

[25396] also doesn't work on Windows.

In my case, the values for unit tests were:

$_SERVER['SCRIPT_FILENAME']: S:\\usr\\local\\php5\phpunit
ABSPATH: S:\home\wordpress\develop/src/
Resulting URL: http://example.orgS:\home\wordpress\develop/src

When trying to set up a new install:

$_SERVER['SCRIPT_FILENAME']: S:/home/wordpress/trunk/index.php
ABSPATH: S:\home\wordpress\trunk/
Resulting URL: http://trunk.wordpressS:\home\wordpress\trunk

Visiting http://trunk.wordpress results in a failed redirect to http://trunk.wordpressS:homewordpresstrunk/wp-admin/install.php.

25317.patch fixes both the unit tests and the installation for me by normalizing slashes and providing a fallback if none of the strpos() conditions are met.

Last edited 7 months ago by SergeyBiryukov (previous) (diff)

comment:4 follow-up: dd327 months ago

attachment:25317.patch

That last fallback case might as well be $path = $_SERVER['REQUEST_URI'], since wp-admin and wp-login are caught in the first block.

I would love it if we could simply fix ABSPATH once-and-for-all, sanitizing the slashes at define time..

What we need to get is a set of unit tests here, except, the fact we're dealing with constants kind of makes that almost impossible with the current testing methodology. thoughts?

Perhaps those with scenario's that break can post a set of $_SERVER data that could be used in a test? (SCRIPT_FILENAME, PHP_SELF, REQUEST_URI, ABSPATH, etc.)

SergeyBiryukov7 months ago

comment:5 in reply to: ↑ 4 SergeyBiryukov7 months ago

Replying to dd32:

That last fallback case might as well be $path = $_SERVER['REQUEST_URI'], since wp-admin and wp-login are caught in the first block.

Thanks, updated.

Perhaps those with scenario's that break can post a set of $_SERVER data that could be used in a test? (SCRIPT_FILENAME, PHP_SELF, REQUEST_URI, ABSPATH, etc.)

My data, when running unit tests:

$_SERVER['SCRIPT_FILENAME']: S:\\usr\\local\\php5\\phpunit
$_SERVER['PHP_SELF']: /index.php
$_SERVER['REQUEST_URI']:
ABSPATH: S:\home\wordpress\develop/src/

When setting up a new install:

$_SERVER['SCRIPT_FILENAME']: S:/home/wordpress/trunk/index.php
$_SERVER['PHP_SELF']: /index.php
$_SERVER['REQUEST_URI']: /
ABSPATH: S:\home\wordpress\trunk/

comment:6 dd327 months ago

In 25436:

Account for Windows and CLI instances in wp_guess_url(). Props SergeyBiryukov. See #25317

comment:7 atimmer7 months ago

I can confirm trunk now works on my installation.

When running tests:

$_SERVER['SCRIPT_FILENAME']: /home/anton/sites/wordpress-trunk/repo/tests/phpunit/includes/install.php
$_SERVER['PHP_SELF']: /index.php
$_SERVER['REQUEST_URI']:
ABSPATH: /home/anton/sites/wordpress-trunk/repo/src/

In the browser:

$_SERVER['SCRIPT_FILENAME']: /home/anton/sites/wordpress-trunk/www/index.php
$_SERVER['PHP_SELF']: /index.php
$_SERVER['REQUEST_URI']: /
ABSPATH: /home/anton/sites/wordpress-trunk/repo/src/

comment:8 follow-up: dd327 months ago

So after thinking about it, the only way we could really unit test this would be to change
$abspath_fix = str_replace( '\\', '/', ABSPATH );
to something like this:
$abspath_fix = str_replace( '\\', '/', ( isset( $_SERVER['ABSPATH'] ) ? $_SERVER['ABSPATH'] : ABSPATH ) );
or to put a filter around ABSPATH.

It'd be nice to have a consistent way to unit test things that use ABSPATH, for example, for WP_Filesystem I had to override the abspath() method in the Mock FS handler to accept an optional path for ABSPATH for use during the tests.
Passing an parameter in works for Unit-test-only functions, but not for core functions, leading me to the $_SERVER hack above.

comment:9 dd327 months ago

  • Keywords needs-unit-tests added; has-patch removed

comment:10 in reply to: ↑ 8 ; follow-up: nacin7 months ago

  • Keywords close added
  • Priority changed from highest omg bbq to normal
  • Severity changed from blocker to normal

Replying to dd32:

It'd be nice to have a consistent way to unit test things that use ABSPATH, for example, for WP_Filesystem I had to override the abspath() method in the Mock FS handler to accept an optional path for ABSPATH for use during the tests.

Usually mocking the class is exactly how testing would occur.

Would $_ENV make more sense than $_SERVER? I'm not sure; just throwing it out there.

I think we can safely close this as fixed, leaving needs-unit-tests of course. Am I correct that this is handled?

comment:11 in reply to: ↑ 10 dd326 months ago

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

Replying to nacin:

Would $_ENV make more sense than $_SERVER? I'm not sure; just throwing it out there.

$_ENV would probably make sense here I agree. (I forgot about that super global)

I think we can safely close this as fixed, leaving needs-unit-tests of course. Am I correct that this is handled?

Correct (AFAIK)

comment:12 SergeyBiryukov5 months ago

  • Version changed from trunk to 3.7
Note: See TracTickets for help on using tickets.