Make WordPress Core

Opened 17 years ago

Closed 11 years ago

#7361 closed defect (bug) (wontfix)

Fixes for wp-app with PHP-CGI

Reported by: yonosoytu's profile yonosoytu Owned by: markjaquith's profile markjaquith
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: AtomPub Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

I was trying to use Atom Publishing Protocol with my blog I a have found some quirks and bugs.

My blog is hosted at Dreamhost, and, as far as I know, is using Apache 2.0.61 and PHP 5.2.6 as CGI. My first problem was, obviously, the authentication, but neither the solutions from the Codex or this other blog post worked. Reserching seems like PHP5 as CGI doesn't forward HTTP_AUTHORIZATION header, and nothing seems to change its mind. But it seems to forward REMOTE_USER as REDIRECT_REMOTE_USER. With my patch and this code in the .htaccess the authorization seems to work:

RewriteCond %{HTTP:Authorization} !^$
RewriteRule wp-app.php wp-app.php [E=REMOTE_USER:%{HTTP:Authorization},QSA,L]

The other patch is to fix a bug in the wp-app.php code. It seems that when working with PHP as CGI, the "Status:" header needs to follow a specific format, with the number, and then the reason. The actual code (from Subversion, but it's there at least from version 2.5.1) do not send the number, making CGI/PHP/Apache to return with a "500 Internal Server Error" instead the "401 Credentials Requiered", confusing Atom clients. My other patch fixes this.

Attachments (5)

001-fix-cgi-status-bug.patch (474 bytes) - added by yonosoytu 17 years ago.
Fix for sending "Status:" header with the correct format so CGI do no send 500 response.
002-use-redirect_remote_user-for-authorization.patch (602 bytes) - added by yonosoytu 17 years ago.
Enhancement to use REMOTE_USER/REDIRECT_REMOTE_USER for authentication.
status-header.diff (664 bytes) - added by tenpura 16 years ago.
Fixes HTTP status headers of PHP-CGI.
status-header-2.diff (574 bytes) - added by tenpura 15 years ago.
Patch using a constant
status-header-3.diff (565 bytes) - added by nacin 15 years ago.
Patch using constant, checking == not ===

Download all attachments as: .zip

Change History (36)

@yonosoytu
17 years ago

Fix for sending "Status:" header with the correct format so CGI do no send 500 response.

@yonosoytu
17 years ago

Enhancement to use REMOTE_USER/REDIRECT_REMOTE_USER for authentication.

#1 @ionfish
17 years ago

  • Cc josephscott rubys added

#2 @ryan
16 years ago

(In [9861]) Send correct status. Props yonosoytu. see #7361

#3 @jacobsantos
16 years ago

See also #8226.

New patch should also be committed.

#4 @ryan
16 years ago

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

(In [9951]) Allow use of REMOTE_USER/REDIRECT_REMOTE_USER for authentication. Props yonosoytu. fixes #7361

#5 @tenpura
16 years ago

  • Milestone changed from 2.7 to 2.8
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version changed from 2.5.1 to 2.8

When running WordPress on a server with PHP as CGI, the HTTP status header will not be correctly set.

Seeing is believing, please compare the status headers of these pages.
http://test.eastcoder.com/wp/trunk/without-the-patch

http://test.eastcoder.com/wp/trunk/with-the-patch

Related ticket: #8226

@tenpura
16 years ago

Fixes HTTP status headers of PHP-CGI.

#6 @Denis-de-Bernardy
16 years ago

  • Keywords needs-testing added

#7 @Denis-de-Bernardy
16 years ago

  • Component changed from General to XML-RPC

#8 @josephscott
16 years ago

  • Component changed from XML-RPC to AtomPub

#9 @Denis-de-Bernardy
16 years ago

  • Milestone changed from 2.8 to Future Release

I can already hear westi requiring unit tests on this one...

#10 @Denis-de-Bernardy
16 years ago

  • Milestone changed from Future Release to 2.9

#11 @Denis-de-Bernardy
16 years ago

  • Keywords needs-unit-tests added; needs-testing removed

patch still applies clean

#12 follow-up: @tenpura
15 years ago

It seems

if('cgi' == substr(strtolower(php_sapi_name()), 0, 3)) 

is insufficient to be a solid identifier.

References:

http://drupal.org/node/175855

http://core.trac.wordpress.org/ticket/3528

How about using a constant to let user decide applying the code?

2.9 still needs a remedy for this issue.

@tenpura
15 years ago

Patch using a constant

@nacin
15 years ago

Patch using constant, checking == not ===

#13 @nacin
15 years ago

  • Keywords needs-unit-tests removed

Especially considering http://bugs.php.net/bug.php?id=20943 et al., the constant remedy appears to be a no-harm bandage that can go into 2.9 until we can come up with a solid remedy.

#14 @westi
15 years ago

  • Keywords needs-info added; has-patch removed
  • Milestone changed from 2.9 to Future Release

This ticket has already had a number of patches applied and it is now not clear what the remaining issue is and how to reproduce it.

Could someone please summarise the information so that the patch can be reviewed and the best solution resolved.

Moving to Future Release until that is done.

#15 @tenpura
15 years ago

  • Keywords has-patch added; needs-info removed
  • Milestone changed from Future Release to 2.9
  • Version changed from 2.8 to 2.9

westi,

Remaining issue:
When running WordPress on a server with PHP as CGI, the HTTP status header will not be correctly set.
(Recent patches are originally about the issue of #8226. I know it is confusing.)

How to reproduce it:

  1. Run WordPress on a server with PHP as CGI.
  2. Access a page that does not exist so that "404 OK" HTTP status header is returned.

Demo:
This URL returns "404 OK".
http://test.eastcoder.com/wp/trunk/without-the-patch
This URL returns "404 Not Found" correctly with the actual patch.
http://test.eastcoder.com/wp/trunk/with-the-patch

I hope this is a good enough explanation.

#16 @westi
15 years ago

  • Keywords 2nd-opinion added

Ok I have tested locally with php using fast-cgi with apache2 and I cannot reproduce the issue you are seeing.

The patch appears to apply fine without causing any issues when not running as fast cgi so I think it should be safe to apply however I would like to get a 2nd-opinion before committing as I cannot reproduce the issue and I know we have had issues getting this working correctly in the past.

#17 @tenpura
15 years ago

I have tested this with DreamHost FastCGI environment and I cannot reproduce the issue either.
Maybe this is a regular CGI specific problem, not FastCGI.

#18 @westi
15 years ago

  • Cc markjaquith added

Ok some related tickets here:

#3528 - Looks like we used to send the Status: header for non-cgi setups - http://core.trac.wordpress.org/changeset/4684/trunk/wp-includes/functions.php

#6779 - Change to wp_redirect to use the argument of header which takes a status - http://core.trac.wordpress.org/changeset/12358

Also search through all calls to status_header for redirects we don't send it for cgi-fcgi sapi where as all other calls to status_header are unconditional - I wonder if we should be rolling the cgi check into status header or whether the check in wp_redirect is specific to Location headers

What server software / php version are you running which exhibits the issue?

#19 @tenpura
15 years ago

Setting two lines together like below seems to output a proper status header with various server setups.

header('Status: 403 Forbidden', true, 403);
header('HTTP/1.1 403 Forbidden', true, 403);

I have tested with these and their status headers look ok so far.

  • Apache 1.3.41, PHP 4.4.8, php_sapi: cgi (This one exhibits the issue as seen in demo.)
  • Apache version unknown, PHP 5.2.9, php_sapi: cgi-fcgi
  • Apache version unknown, PHP 5.2.11, php_sapi: apache

One side effect of this is that unnecessary "Status: XXX" line is added to headers under non cgi setups. However, it appears to be harmless.

HTTP/1.1 403 Forbidden
Date: Mon, 14 Dec 2009 07:53:12 GMT
Server: Apache
Status: 403 Forbidden // added line
Keep-Alive: timeout=15, max=100
Connection: Keep-Alive
Content-Type: text/html
Content-Length: 3

#20 @markjaquith
15 years ago

  • Milestone changed from 2.9 to 3.0
  • Owner changed from anonymous to markjaquith
  • Status changed from reopened to assigned

I don't want to change this right before RC. This is a "fix early in the dev cycle" bug.

Also, note that even the server sending the incorrect header is sending the correct status code. the part that comes after the status code is for humans — machines are supposed to read the code.

#21 @Denis-de-Bernardy
15 years ago

wasn't this fixed already in r12358 / #6779?

#22 @Denis-de-Bernardy
15 years ago

oh, nevermind, I misread the ticket.

#23 @hakre
15 years ago

Asking those who provided patches / discuessed: Can we draw a conclusion line here and summarize this up? Looks like a valid ticket to me and it would be nice to have this a bit more compact / easy to test & commit. Something to test? Just let me know the steps.

#24 follow-up: @hakre
15 years ago

How to reproduce?

#25 in reply to: ↑ 24 @nacin
15 years ago

Replying to hakre:

How to reproduce?

http://core.trac.wordpress.org/ticket/7361#comment:15 and the four that follow it.

#26 @hakre
15 years ago

I am not able to reproduce against current trunk on CGI nor FCGI. Since this ticket was opened month ago I think it's valuable to think about that the third parameter to the header function was added in PHP 4.3. That third parameter is now used in the codebase (at least at the places I had been looking into, I did not a regex search right now) and it is related to the Status Code, I pretty much assume that this problem is now fixed but it might not have been for older PHP version. Can that be the case here?

Which PHP Version is used on that testbed where it's said it's reproduceable? Is it updated to 2.9.1 or current trunk?

#27 in reply to: ↑ 12 @hakre
15 years ago

Replying to tenpura:

It seems

if('cgi' == substr(strtolower(php_sapi_name()), 0, 3)) 

is insufficient to be a solid identifier.

References:

http://drupal.org/node/175855

http://core.trac.wordpress.org/ticket/3528

How about using a constant to let user decide applying the code?

2.9 still needs a remedy for this issue.

This code is used here:
php_sapi_name() == 'cgi-fcgi' Related: #10187

suggestions welcome. I can do sniffing over the current codebase then.

#28 @dd32
15 years ago

  • Keywords early added
  • Milestone changed from 3.0 to 3.1

Close to RC's and beta's, Not the time to be changing this again, Punting to 3.1 early for dealing with

#29 @nacin
14 years ago

  • Keywords early removed
  • Milestone changed from Awaiting Triage to Future Release

#30 @kurtpayne
12 years ago

  • Cc kurtpayne added

Is this still valid?

A related php bug #27345 notes that this behavior is now covered by the cgi.rfc2616_headers php.ini setting

#31 @nacin
11 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

As AtomPub was removed from core in 2012, see #21866. There is a plugin for it here: http://wordpress.org/plugins/atom-publishing-protocol/.

I doubt anyone really interfaces with WordPress via AtomPub anymore. Did anyone ever truly use it? I heard that when WordPress.com had it enabled, a few dozen posts came through it a day, all from few clients.

So realistically, AtomPub is essentially considered "end of life" to us. This plugin was merely done for backwards compatibility.

Closing this ticket as wontfix.

Note: See TracTickets for help on using tickets.