WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#5682 closed defect (bug) (fixed)

Clean permalinks and mod_rewrite support for IIS

Reported by: man999 Owned by: ryan
Milestone: 2.8.1 Priority: normal
Severity: normal Version: 2.5
Component: Permalinks Keywords: has-patch commit
Focuses: Cc:

Description

Hello team,

In wp-settings.php you have recently introduced a very important improvement that allows Wordpress clean permalinks to work with the two major url rewriting tools, IIS Mod-Rewrite, and ISAPI_Rewrite.

However the condition

if ( empty( $_SERVER['REQUEST_URI'] ) )

in line 29 does not always work properly. PHP 5.2.x always sets the server variable REQUEST_URI, preventing the execution of the supportive code for IIS url rewriting.

My suggestion is to replace this condition with the following one:

if ( empty( $_SERVER['REQUEST_URI'] ) || substr( $_SERVER['SERVER_SOFTWARE'], 0, 13 ) == 'Microsoft-IIS' )

This condition, will accuratelly recognize IIS. I have tested this on IIS versions 5, 6, and 7, and it works perfect on all of them.

Regards

Attachments (5)

5682.r6635.diff (469 bytes) - added by hansengel 7 years ago.
A patch of man999's solution—test that REQUEST_URI isn't set or that the current server is indeed IIS
5682.patch (474 bytes) - added by peaceablewhale 6 years ago.
5682.function-check.diff (445 bytes) - added by filosofo 6 years ago.
5682_fcgi.patch (579 bytes) - added by ruslany 6 years ago.
Extra condition to check for php_sapi_name()
5682_fcgi.2.patch (583 bytes) - added by ruslany 6 years ago.
Updated patch as per ddebernardy suggestions

Download all attachments as: .zip

Change History (39)

comment:1 @hansengel7 years ago

  • Keywords needs-patch added
  • Milestone changed from 2.6 to 2.5
  • Version set to 2.5

@hansengel7 years ago

A patch of man999's solution—test that REQUEST_URI isn't set or that the current server is indeed IIS

comment:2 @hansengel7 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

comment:3 @ffemtcj7 years ago

  • Milestone changed from 2.5 to 2.6

comment:4 @Denis-de-Bernardy6 years ago

  • Component changed from General to Permalinks
  • Owner changed from anonymous to ryan

comment:5 @Denis-de-Bernardy6 years ago

  • Keywords reporter-feedback added; needs-testing removed

PHP 5.2.x always sets the server variable REQUEST_URI

Curious to know what it sets it to. It can't be to something invalid, can it?

comment:6 @Denis-de-Bernardy6 years ago

  • Keywords reporter-feedback removed

an empty string based on #100169

comment:7 @Denis-de-Bernardy6 years ago

#10169 even. it also suggests to use $_SERVERHTTP_X_ORIGINAL_URL? when available.

comment:8 @peaceablewhale6 years ago

  • Keywords needs-testing added
  • Milestone changed from 2.9 to 2.8.1
  • Type changed from enhancement to defect (bug)

Targetting 2.8.1 due to #8974.

comment:9 @Denis-de-Bernardy6 years ago

sadly, $is_IIS is defined further down in wp-settings.php so can't be used here.

comment:10 @peaceablewhale6 years ago

Oops... thanks for the note. Use $_SERVERSERVER_SOFTWARE?,'Microsoft-IIS')!==false then...

@peaceablewhale6 years ago

comment:11 @Denis-de-Bernardy6 years ago

  • Keywords commit added; needs-testing removed

comment:12 follow-up: @filosofo6 years ago

Why not just test whether IIS-specific PHP functions, such as iis_add_server(), exist?

comment:13 in reply to: ↑ 12 @peaceablewhale6 years ago

Replying to filosofo:

Why not just test whether IIS-specific PHP functions, such as iis_add_server(), exist?

because those functions will not work when PHP is installed on IIS via FastCGI.

comment:14 @Denis-de-Bernardy6 years ago

commit is for 5682.patch

comment:15 @peaceablewhale6 years ago

Actually, both this and #10169 do not affect IIS 7.5. Should we exclude it from the patch? Mainly a performance concern.

comment:16 @Denis-de-Bernardy6 years ago

I don't think so. We'd end up losing on the performance front for all other IIS users, and potentially others (version_compare() is very slow).

comment:17 @peaceablewhale6 years ago

How about using (preg_match("/Microsoft-IIS\/(\d+(?:\.\d+)?)/",$_SERVERSERVER_SOFTWARE?,$iis_version)===1 && $iis_version[1]<=7)?

comment:18 @Denis-de-Bernardy6 years ago

preg_match is going to slow things down

comment:19 @ruslany6 years ago

  • Keywords close added

I suggest resolve this bug as won't fix. When PHP is run on IIS 5.1 and IIS 6.0 with FastCGI (which is the recommended way to run PHP on IIS), then the REQUEST_URI will be set correctly. When PHP is run on IIS 7 with FastCGI and there is a URL Rewrite Module installed then the REQUEST_URI will be set correctly as well. So the only cases when it will not be set are: 1. When PHP is run as ISAPI - this is not recommended way of running PHP and it should be discouraged 2. When PHP is run on IIS 7 with FastCGI but there is no FastCGI patch applied. This will be extremely rare case as the main reason REQUEST_URI is used is to support pretty permalinks. Those require IIS 7 URL rewrite module installed. When that module is installed it also installs the FastCGI patch for REQUEST_URI.

comment:20 @Denis-de-Bernardy6 years ago

  • Keywords close removed

As much as I agree with your arguments, the general trend here is that this should still get fixed as it'll affect some users.

comment:21 @ruslany6 years ago

There is another problem with this patch. With this patch the WordPress will start using HTTP_X_ORIGINAL_URL server variable on IIS instead of the proper REQUEST_URI. Using that server variable is not recommended by IIS team as its value can be controlled by client by setting the HTTP header "x-original-url". This may be a potential security issue. Using of REQUEST_URI is recommended because it is set by IIS only and cannot be controlled by client.

IIS team has deliberately added REQUEST_URI support to all IIS versions so that workarounds like this one could be avoided.

comment:22 @Denis-de-Bernardy6 years ago

  • Keywords needs-patch added; has-patch commit removed

ah, that is a much better point. needs another patch then. :-|

@peaceablewhale: what does the $_SERVER[SERVER_SOFTWARE] look like? a regexp sounds in order then, as in:

preg_match("/Microsoft-IIS\/(\d+(?:\.\d+)?)/", ...

note the anchor at the beginning (). Without it, preg_match won't scale.

comment:23 @Denis-de-Bernardy6 years ago

preg_match("/^Microsoft-IIS\/(\d+(?:\.\d+)?)/", ...


even

@ruslany6 years ago

Extra condition to check for php_sapi_name()

comment:24 @ruslany6 years ago

I think the patch 5682_fcgi.patch may work better. It still uses strpos instead of regex, which should be faster. Plus it checks that the PHP is not run via FastCGI, as with FastCGI the REQUEST_URI is set correctly on all versions of IIS.

comment:25 @Denis-de-Bernardy6 years ago

Contrary to what the php docs say, anchored regex is *much* faster than strpos on a highly loaded server. So seriously advising an anchored regex if we can get one in there.

comment:26 @Denis-de-Bernardy6 years ago

for the fasted test, you'd go:

empty(_$SERVER['REQUEST_URI']) || php_sapi_name() != 'cgi-fcgi' && preg_match(/^...)

@ruslany6 years ago

Updated patch as per ddebernardy suggestions

comment:27 @ruslany6 years ago

The main reason regex was proposed in a first place was to extract the version information from the SERVER_SOFTWARE variable. As there is no need to do that, strpos seemed to do the job. However, if you say that anchored regex is faster that's fine by me.

I've updated the patch (5682_fcgi.2.patch) to use preg_match and also re-arranged the condition operands for faster test as per suggestion from ddebernardy.

comment:28 @Denis-de-Bernardy6 years ago

  • Keywords has-patch commit added; needs-patch removed

I'm +1 for the latest patch

comment:29 @peaceablewhale6 years ago

+1 to the latest patch too. However, is it better to add a () to the second part of the new statement?

empty( $_SERVER['REQUEST_URI'] ) || ( php_sapi_name() != 'cgi-fcgi' && preg_match( '/^Microsoft-IIS\//' , $_SERVER['SERVER_SOFTWARE'] ) > 0 )

comment:30 @Denis-de-Bernardy6 years ago

for readability's sake only. it's not needed in php.

false
true && true = false ( true && true ) = true

comment:31 @Denis-de-Bernardy6 years ago

doh, trac got rid of the or thingies.

comment:32 @peaceablewhale6 years ago

Improving readability should be good :)

Perhaps we can leave this to the committer.

comment:33 @azaozz6 years ago

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

(In [11661]) Improved $_SERVERREQUEST_URI? test for IIS, props ruslany, fixes #5682 for 2.8

comment:34 @azaozz6 years ago

(In [11662]) Improved $_SERVERREQUEST_URI? test for IIS, props ruslany, fixes #5682 for trunk

Note: See TracTickets for help on using tickets.