Re: svn commit: r1357986 - /httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c
On Thu, Oct 3, 2013 at 1:06 AM, Chris Darroch chr...@pearsoncmg.com wrote: Chris Darroch wrote: The intent of r1357986 was to deal with a particular, wonky sub-case, when the Authorizer returns 200 (so the spec paragraph doesn't apply in this case, as it's a 200 OK response), but adds a Location header with a relative (not absolute) path. In this case, 2.3.7 and previous will run the sub-request and return whatever comes out of that -- sometimes munging the end result as a consequence. (Note that a 200 with an absolute URL in a Location header just produces a 401 response.) Actually, I have to take back that last parenthetical note -- with 2.3.7, a 200 + absolute Location URL produces a 302 + Location header, and with trunk, it produces a 401 like other 200 + Location header cases. That might be, I suppose, considered a regression ... let me think on it a bit. It's not covered by the spec case you mention, since the script is returning 200. The 2.3.7 behaviour is inconsistent depending on the nature of the URL in the Location header, given a 200. Trunk makes that behaviour consistent, but perhaps that's a regression? Hmm. The app is out of spec either way. I think the trunk behavior is better. Chris. -- GPG Key ID: 088335A9 GPG Key Fingerprint: 86CD 3297 7493 75BC F820 6715 F54F E648 0883 35A9 -- Born in Roswell... married an alien... http://emptyhammock.com/
Re: svn commit: r1357986 - /httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c
Jeff Trawick wrote: The app is out of spec either way. I think the trunk behavior is better. I'd agree on both counts (the latter IMHO, of course). For reference, here's a breakdown of 2.3.7 vs. trunk behaviour for Authorizers: Authorizer response2.3.7 trunk ==== = 200proceed proceed 200 + rel Location 200 + munged out + err 401 200 + bad rel Location 404 + munged err + err 401 200 + abs Location 302 + Location 401 all other 401 401 I'll try to run some quick smoke tests on your 2.3.9 over the weekend; thanks again for pushing this along. Chris. -- GPG Key ID: 088335A9 GPG Key Fingerprint: 86CD 3297 7493 75BC F820 6715 F54F E648 0883 35A9
Re: svn commit: r1357986 - /httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c
Jeff Trawick wrote: URL: http://svn.apache.org/viewvc?rev=1357986view=rev http://svn.apache.org/viewvc?rev=1357986view=rev Log: Avoid internal sub-requests and processing of Location headers when in FCGI_AUTHORIZER mode, as the mod_fcgid_authenticator(), etc. hook functions report an error if the script returned a Location header and redirections are nonsensical in this mode. Previously, the handle_request_ipc() and handle_request() functions would examine this header when in FCGI_AUTHORIZER mode and then possibly execute an internal sub-request, which has no particular use, as its return value is ignored and its output may conflict with that of the actual content generation phase. From the FastCGI spec (6.3): For Authorizer response status values other than 200 (OK), the Web server denies access and sends the response status, headers, and content back to the HTTP client. I was initially confused when looking at this commit (nothing like reviewing one year later) wondering if it broke this requirement, but AFAICT 2.3.7 didn't support the feature anyway, so no regression. (Some iff statements in this code are what control it.) Yes, I believe that's correct (it's been a while). It's a really good question and worth having a second pair of eyes. I did a smidge of testing today, comparing 2.3.7 and trunk, with a test Authorizer that returns things like 403, 500, etc. I believe there's no difference for most of these cases; the 2.3.7 behaviour is to return a 401, which is still the case. (This may not follow the spec, as you point out, but is the existing behaviour; I'll double-check on that tomorrow.) The intent of r1357986 was to deal with a particular, wonky sub-case, when the Authorizer returns 200 (so the spec paragraph doesn't apply in this case, as it's a 200 OK response), but adds a Location header with a relative (not absolute) path. In this case, 2.3.7 and previous will run the sub-request and return whatever comes out of that -- sometimes munging the end result as a consequence. (Note that a 200 with an absolute URL in a Location header just produces a 401 response.) For example, if the Authorizer returns Location: /cgi-bin/printenv and a 200, you get something strange like: HTTP/1.1 200 OK ... CONTEXT_PREFIX=/cgi-bin/ ... SERVER_SOFTWARE=Apache/2.5.0-dev (Unix) mod_fcgid/2.3.7 !DOCTYPE HTML PUBLIC -//IETF//DTD HTML 2.0//EN htmlhead title200 Unknown Reason/title /headbody h1Unknown Reason/h1 ... Or if the Location is bad (say, Location: /cgi-bin/printenv.FOO) then you get a 404 with a 200 Unknown Reason glued on: HTTP/1.1 404 Not Found ... !DOCTYPE HTML PUBLIC -//IETF//DTD HTML 2.0//EN htmlhead title404 Not Found/title /headbody h1Not Found/h1 pThe requested URL /cgi-bin/printenv.FOO was not found on this server./p /body/html !DOCTYPE HTML PUBLIC -//IETF//DTD HTML 2.0//EN htmlhead title200 Unknown Reason/title /headbody So r1357986 is just trying to handle those 200 OK + Location: /... header cases. If it does more than that, we should review further, but that was the only intent ... IIRC after about a year or so. Chris. -- GPG Key ID: 088335A9 GPG Key Fingerprint: 86CD 3297 7493 75BC F820 6715 F54F E648 0883 35A9
Re: svn commit: r1357986 - /httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c
Chris Darroch wrote: The intent of r1357986 was to deal with a particular, wonky sub-case, when the Authorizer returns 200 (so the spec paragraph doesn't apply in this case, as it's a 200 OK response), but adds a Location header with a relative (not absolute) path. In this case, 2.3.7 and previous will run the sub-request and return whatever comes out of that -- sometimes munging the end result as a consequence. (Note that a 200 with an absolute URL in a Location header just produces a 401 response.) Actually, I have to take back that last parenthetical note -- with 2.3.7, a 200 + absolute Location URL produces a 302 + Location header, and with trunk, it produces a 401 like other 200 + Location header cases. That might be, I suppose, considered a regression ... let me think on it a bit. It's not covered by the spec case you mention, since the script is returning 200. The 2.3.7 behaviour is inconsistent depending on the nature of the URL in the Location header, given a 200. Trunk makes that behaviour consistent, but perhaps that's a regression? Hmm. Chris. -- GPG Key ID: 088335A9 GPG Key Fingerprint: 86CD 3297 7493 75BC F820 6715 F54F E648 0883 35A9
Re: svn commit: r1357986 - /httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c
On Thu, Jul 5, 2012 at 7:01 PM, chr...@apache.org wrote:
Author: chrisd
Date: Thu Jul 5 23:01:09 2012
New Revision: 1357986
URL: http://svn.apache.org/viewvc?rev=1357986view=rev
Log:
Avoid internal sub-requests and processing of Location headers when
in FCGI_AUTHORIZER mode, as the mod_fcgid_authenticator(), etc. hook
functions report an error if the script returned a Location header and
redirections are nonsensical in this mode.
Previously, the handle_request_ipc() and handle_request() functions would
examine this header when in FCGI_AUTHORIZER mode and then possibly execute
an internal sub-request, which has no particular use, as its return value
is ignored and its output may conflict with that of the actual content
generation phase.
From the FastCGI spec (6.3):
For Authorizer response status values other than 200 (OK), the Web
server denies access and sends the response status, headers, and content
back to the HTTP client.
I was initially confused when looking at this commit (nothing like
reviewing one year later) wondering if it broke this requirement, but
AFAICT 2.3.7 didn't support the feature anyway, so no regression. (Some
iff statements in this code are what control it.)
Modified:
httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c
Modified: httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c
URL:
http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c?rev=1357986r1=1357985r2=1357986view=diff
==
--- httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c (original)
+++ httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c Thu Jul 5 23:01:09
2012
@@ -320,6 +320,10 @@ handle_request_ipc(request_rec *r, int r
return cond_status;
}
+if (role == FCGI_AUTHORIZER) {
+return cond_status;
+}
+
/* Check redirect */
location = apr_table_get(r-headers_out, Location);
@@ -347,9 +351,8 @@ handle_request_ipc(request_rec *r, int r
}
/* Now pass to output filter */
-if (role == FCGI_RESPONDER
- (rv = ap_pass_brigade(r-output_filters,
- brigade_stdout)) != APR_SUCCESS) {
+if ((rv = ap_pass_brigade(r-output_filters,
+ brigade_stdout)) != APR_SUCCESS) {
if (!APR_STATUS_IS_ECONNABORTED(rv)) {
ap_log_rerror(APLOG_MARK, APLOG_WARNING, rv, r,
mod_fcgid: ap_pass_brigade failed in
--
Born in Roswell... married an alien...
http://emptyhammock.com/
