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

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).

>> 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,

Alex.
Received on Mon Nov 29 2010 - 18:44:37 MST