Re: [PATCH] Unknown cfg function

On 31/07/2013 6:47 a.m., Tsantilas Christos wrote:
> On 07/30/2013 09:17 PM, Amos Jeffries wrote:
>>> However Amos refers to an other case. For the following line:
>>> "Simple Tokens"
>>> we may want to retrieve the token "Simple
>>>
>>> Do we have any example where this is required? (Not for regex, for regex
>>> we have an exception...)
>> In most places where we accept header values in the legacy parser.
> Well the request_header_add already understand quoted strings.
>
> The parse_http_header_replace should use the
> ConfigParser::NextQuotedOrToEol.
> I just show that there is a problem here. It will work as is. But the
> code is not correct....

I don't think it will work as-is. If I were to be replacing ETag headers
they contain a single value with "" around it. Or any other header
holding a quoted-string field value, this is more likely with custom
headers. Then the NextToken it is currently using will corrupt the
resulting HTTP traffic.

> The other functions hae only header names...

The ACLs doing matching on header values are all regex I think.

>
>> I have seen a few people who have things like "quoted string" for auth
>> realm, and since the old squid.conf sent the quotes out as-is they ended
>> up with helpers and browsers security records using the quoted form.
>> There might be any amount of unknown pain ahead if we suddenly start
>> stripping quotes off strings in the backward compatible parse mode.
> The realm uses the parse_eol which uses the
> ConfigParser::NextQuotedOrToEol method which handles such cases....

The point was that the "" are now being stripped whereas before they
were not.

When the new parse mode is enabled that is fine. When the mode is
disabled we are likely breaking things.

If there is anything important which requires those quotes to be sent in
the HTTP request then the change will break it. The obvious case is
Digest auth where the realm is hashed into the nonces as-is and
federated remote servers may store that hashed value for re-use. If
Squid is no longer sending the same realm hash logins will fail in an
almost undetectable way.
There are people also doing similar things using the Basic auth realm in
nasty ways so I would not be surprised if a quote changing there broken
stuff for them as well.

What I am hearing is that ConfigParser::NextQuotedOrToEol() is missing a
check for ConfigParser::RecognizeQuotedValues to determine whether it
treats quoted values as the "ToEol" case. Anything which is using it
that used to have quoted-string support may need to preserve
RecognizeQuotedValues, set it to true, run the function then reset it to
the old value.

>
>>
>>>
>>>>
>>>>>> For the new style if someone want to use '(' character on a regex for
>>>>>> example, he should use quotes:
>>>>>> 'test(.*/)'
>>>>>> "test(.*/)"
>>>> Double quotes do not work well because REs use backslashes of their own.
>>>> It becomes too messy as Amos noted above. I think we should not support
>>>> REs in a new parser until we add Perl-like RE quoting (or better).
>>> OK. This is something we can do it.
>>> We can add a method ConfigParser::NextWord which will return a raw word
>>> which will consist by any non whitespace character. Whitespaces can be
>>> escaped.
>>> Or use an On/Off global variable which enables/disables for the next
>>> token this behaviour....
>> Hmm. Might work.
>>
>> I have in mind to shuffle the ConfigParser methods into groups. One for
>> the parser mechanics and one group for the type-specific GetX() methods.
>> Some of those methods are provided in ConfigParser already, the rest are
>> mixed up with src/Parsing.cc.
>>
>> A ConfigParser::RegexPattern() method should be perfectly fine to add.
>>
> This is looks good.

Amos
Received on Tue Jul 30 2013 - 20:15:12 MDT