You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jeff Trawick <tr...@gmail.com> on 2013/10/02 00:43:58 UTC
Re: svn commit: r1357986 - /httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c
On Thu, Jul 5, 2012 at 7:01 PM, <ch...@apache.org> wrote:
> Author: chrisd
> Date: Thu Jul 5 23:01:09 2012
> New Revision: 1357986
>
> URL: http://svn.apache.org/viewvc?rev=1357986&view=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=1357986&r1=1357985&r2=1357986&view=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/
Re: svn commit: r1357986 - /httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c
Posted by Chris Darroch <ch...@pearsoncmg.com>.
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 response 2.3.7 trunk
=================== ===== =====
200 proceed 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
Posted by Jeff Trawick <tr...@gmail.com>.
On Thu, Oct 3, 2013 at 1:06 AM, Chris Darroch <ch...@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
Posted by Chris Darroch <ch...@pearsoncmg.com>.
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
Posted by Chris Darroch <ch...@pearsoncmg.com>.
Jeff Trawick wrote:
> URL: http://svn.apache.org/viewvc?rev=1357986&view=rev
> <http://svn.apache.org/viewvc?rev=1357986&view=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">
<html><head>
<title>200 Unknown Reason</title>
</head><body>
<h1>Unknown 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">
<html><head>
<title>404 Not Found</title>
</head><body>
<h1>Not Found</h1>
<p>The requested URL /cgi-bin/printenv.FOO was not found on this server.</p>
</body></html>
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>200 Unknown Reason</title>
</head><body>
====================================
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