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

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.

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

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.9
   Beta testers wanted for 3.2.0.3