Make WordPress Core

Opened 10 years ago

Closed 8 years ago

Last modified 8 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:


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.


Attachments (5)

5682.r6635.diff (469 bytes) - added by hansengel 10 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 8 years ago.
5682.function-check.diff (445 bytes) - added by filosofo 8 years ago.
5682_fcgi.patch (579 bytes) - added by ruslany 8 years ago.
Extra condition to check for php_sapi_name()
5682_fcgi.2.patch (583 bytes) - added by ruslany 8 years ago.
Updated patch as per ddebernardy suggestions

Download all attachments as: .zip

Change History (39)

#1 @hansengel
10 years ago

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

10 years ago

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

#2 @hansengel
10 years ago

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

#3 @ffemtcj
10 years ago

  • Milestone changed from 2.5 to 2.6

#4 @Denis-de-Bernardy
9 years ago

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

#5 @Denis-de-Bernardy
9 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?

#6 @Denis-de-Bernardy
8 years ago

  • Keywords reporter-feedback removed

an empty string based on #100169

#7 @Denis-de-Bernardy
8 years ago

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

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

#9 @Denis-de-Bernardy
8 years ago

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

#10 @peaceablewhale
8 years ago

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

8 years ago

#11 @Denis-de-Bernardy
8 years ago

  • Keywords commit added; needs-testing removed

#12 follow-up: @filosofo
8 years ago

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

#13 in reply to: ↑ 12 @peaceablewhale
8 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.

#14 @Denis-de-Bernardy
8 years ago

commit is for 5682.patch

#15 @peaceablewhale
8 years ago

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

#16 @Denis-de-Bernardy
8 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).

#17 @peaceablewhale
8 years ago

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

#18 @Denis-de-Bernardy
8 years ago

preg_match is going to slow things down

#19 @ruslany
8 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.

#20 @Denis-de-Bernardy
8 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.

#21 @ruslany
8 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.

#22 @Denis-de-Bernardy
8 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.

#23 @Denis-de-Bernardy
8 years ago

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


8 years ago

Extra condition to check for php_sapi_name()

#24 @ruslany
8 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.

#25 @Denis-de-Bernardy
8 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.

#26 @Denis-de-Bernardy
8 years ago

for the fasted test, you'd go:

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

8 years ago

Updated patch as per ddebernardy suggestions

#27 @ruslany
8 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.

#28 @Denis-de-Bernardy
8 years ago

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

I'm +1 for the latest patch

#29 @peaceablewhale
8 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 )

#30 @Denis-de-Bernardy
8 years ago

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

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

#31 @Denis-de-Bernardy
8 years ago

doh, trac got rid of the or thingies.

#32 @peaceablewhale
8 years ago

Improving readability should be good :)

Perhaps we can leave this to the committer.

#33 @azaozz
8 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

#34 @azaozz
8 years ago

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

Note: See TracTickets for help on using tickets.