[squid-dev] [PATCH] adapting 100-Continue / A Bug 4067 fix

[Tsantilas Christos]

On 01/01/2015 01:47 AM, Alex Rousskov wrote:
> On 11/09/2014 02:02 PM, Tsantilas Christos wrote:
>
>>   void
>>   Http::Server::processParsedRequest(ClientSocketContext *context)
>>   {
>> +    if (!buildHttpRequest(context))
>> +        return;
>> +
>> +    if (Config.accessList.forceRequestBodyContinuation) {
>> +        ClientHttpRequest *http = context->http;
>> +        HttpRequest *request = http->request;
>> +        ACLFilledChecklist bodyContinuationCheck(Config.accessList.forceRequestBodyContinuation, request, NULL);
>> +        if (bodyContinuationCheck.fastCheck() == ACCESS_ALLOWED) {
>> +            debugs(33, 5, "Body Continuation forced");
>> +            request->forcedBodyContinuation = true;
>
>
> The HTTP code above sends 100-Continue responses to HTTP GET messages
> unless the admin is very careful with the ACLs. This can be reproduced
> trivially with
>
>    force_request_body_continuation allow all
>
> We should not evaluate force_request_body_continuation if the request
> does not have a body IMO. The force_request_body_continuation
> documentation makes that option specific to upload requests. If you
> agree, please adjust the committed code accordingly.

:-(

I am attaching a patch which fixes this. The patch attached in both 
normal form and "-b" diff option to allow squid developers examine the 
proposed changes.

In my patch I am moving the check for "Expect: 100-Continue" header from 
clientProcessRequest() (located in client_side.cc file), which checks 
for valid Expect header values, to Http::Server::processParsedRequest 
method to minimise the required checks.

  I believe this relocation is valid because this check is needed only 
for HTTP protocol. For FTP protocol the Expect header is generated by 
squid and is not possible to include not supported values.

Alternately we can just check if "Expect: 100-continue" header exist 
inside Http::Server::processParsedRequest.

>
>
> The similar FTP check seems to be inside the upload-specific code and,
> hence, should not need additional "do we expect a body?" guards.

Yep.

>
>
> Thank you,
>
> Alex.
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: HTTP-100-Continue-forceRequestBodyContinuation-bug-diff-b-t1.patch
Type: text/x-patch
Size: 3295 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150107/704709dd/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: HTTP-100-Continue-forceRequestBodyContinuation-bug-t1.patch
Type: text/x-patch
Size: 6871 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150107/704709dd/attachment-0003.bin>