Re: [PATCH] [mk3] deny_info 3xx code support

On 11/30/2010 02:59 AM, Amos Jeffries wrote:
> On 30/11/10 07:44, Alex Rousskov wrote:
>> On 11/23/2010 03:38 AM, Amos Jeffries wrote:
>>> On 20/11/10 07:03, Alex Rousskov wrote:
>>>> On 11/19/2010 07:19 AM, Amos Jeffries wrote:
>>>>> Small improvement on the earlier patch.
>>>
>>> I should say the earlier patch is still needed for 3.1 to permit safe
>>> CONNECT redirects. This improved version is for 3.2+.
>>>
>>>>>
>>>>> This adds:
>>>>>
>>>>> * automatic selection of 307 response status for non-GET/HEAD HTTP/1.1
>>>>> requests.
>>>>> * support for custom 3xx codes (only 300..399 allowed) to be
>>>>> configured
>>>>> on the deny_info URL.
>>>>> For example deny_info 303:http://example.com/ POST
>>>>>
>>>>> While retaining the 302 status for all HTTP/1.0 clients in case
>>>>> they do
>>>>> not support the HTTP/1.1 status.
>>>>>
>>>>> I've omitted automatic selection of 303 status, since it is slightly
>>>>> unclear whether it applies to *all* PUT/POST. Easily added at a later
>>>>> stage anyways.
>>>>>
>>>>>
>>>>> Testing in production shows that Firefox supports 307 redirection. The
>>>>> other popular browsers treat it like 302 still.
>>>>
>>>> Why restrict custom codes to the 3xx range? I have heard requests for
>>>> other codes with deny_info. I realize we are adding Location header
>>>> unconditionally, but it should not hurt if the status code is not 3xx.
>>>
>>> mk2 patch only applies to the redirection form of deny_info. If the code
>>> is not 3xx the browser will not fetch the desired content to display the
>>> page.
>>>
>>> I'm not going to implement a background fetch which is required to use
>>> any other status there. I'm not even certain we have a clean way to do
>>> so from random points in the code yet.
>>
>> Agreed. Background fetching is definitely out of scope.
>>
>>
>>> mk3 patch (attached) adds 4xx and 5xx codes to the ERR_* named template
>>> form of deny_info. 3xx is dodgy on these since we can't provide a
>>> Location: header. Though I'm open to allowing it if we need to cater for
>>> someones actual use-case.
>>
>> I missed the redirection-versus-named-template form distinction. You are
>> on the right path.
>>
>>
>>> I don't think there is a case for allowing 1xx and 2xx is there?
>>
>> No 1xx because they are not "final" responses.
>>
>> 2xx-9xx should be allowed, IMO (same reasoning as before -- do not
>> restrict unless you have to, because there are probably valid use cases
>> for some seemingly unusual things). You can warn if something looks
>> suspicious, of course (e.g., 307 with ERR_* named template, at least
>> until we start supporting Location headers in templates).
>
> 2xx okay, that could be interesting to see a use-case for which does not
> involve serving local files to clients (ie PAC). Will open that range on
> next submission.
>
> 3xx with a named template and 4xx/5xx with a redirect break with a range
> of horrible results to our file loading and output Location URLs. My
> tests ended up with Squid scanning the FS for local files called
> http://blah, redirecting the browser to 404:ERR_ACCESS_DENIED, or
> getting past those with zero-sized replies and crashes when err is
> required to have length.

Ouch! This clearly falls into the "unless you have to" category and
should be restricted. I would also add a source comment so that others
do not try to enable those ranges, thinking it is just a useless
restriction. Thank you for testing this thoroughly.

Alex.

> They are going to take something much more major logic re-plumbing and
> maybe deeper cleanup to get the crossover down to safe enough for just a
> warning. Given the RFC defined use of each status range I did not think
> it worth doing to enable something on the fine edge of non-standard.
>
>>
>>>> P.S. Please consider posting diffs with context depth set to 20. They
>>>> are often easier to review.
>>>
>>> Thanks for the reminder. I've been meaning to ask how you set bzr to
>>> produce those?
>>
>> bzr diff --diff-options='-U 20'
>>
>> It is probably possible to add this to your bzr configuration file.
>>
>> Cheers,
>
> Sweet! thanks :)
>
> Amos
Received on Tue Nov 30 2010 - 16:05:07 MST