[squid-dev] Issue with acl note (without -m) splitting helper tokens containing commas

[Amos Jeffries]

On 21/04/2026 06:59, Alex Rousskov wrote:
> On 2026-04-17 09:08, Andrey K wrote:
> 
>> While working with annotations, I’ve noticed an inconsistency in how 
>> acl note (without the -m option) handles tokens received from helpers 
>> when they contain a comma.
> 
> I think it is important to note that there are several distinct players 
> here, including:
> 
> 1. What annotations the helper sends to Squid.
> 

Which is:

     group="Staff:accountants,lawyers,security"


> 2. How helper response parser converts received helper response into
>     transaction annotations.
> 

Which is:

   1) remove double-quotes
   2) translate \-escapes within quoted-string values
   3) translate %-coded within token values

Nothing more. ',' is not special for that parser.


> 3. How the "note" ACL code interprets transaction annotations.
>     These annotations may come from sources other than a helper.
> 

This is where ',' becomes special as a list delimiter.

The annotation system interprets "a,b,c" as a set of three values, 
stored as a list.

  (I do not understand why we need to store annotations in their 
serialized format in the first place. It is generally a bad design.)


> 
>> According to the documentation, an ACL like this:
>>      acl staff note group Staff:accountants,lawyers,security
>> should match a helper response such as:
>>      group="Staff:accountants,lawyers,security"
> 
> The above implies that helper response parser should not split group=X 
> response fields (using comma as a delimiter). Do we document that 
> anywhere?

The only thing the helper response parser does is remove the DQUOTE 
characters around the value string.

As you noted below, the problem is ACL logic interaction with the 
annotation storage.


> Did our helper response parser ever split such fields in the 
> past?
> 

If it did that was a bug. Prior to the annotations feature we did not 
support key=value-list, only key=value (singular value).



> BTW, 2015 commit 76ee67ac used a very similar example. AFAICT by looking 
> at that code, we did not apply value delimiters by default back then 
> (i.e. when ACL_F_SUBSTRING a.k.a. "-m" flag was not set). The bug was 
> introduced in 2017 commit 4eac3407 that replaced a possibly-nil 
> `flags.delimiters()` with a never-nil `&delimiters.value`.
> 
> The following comment suggests that we missed the fact that using the 
> [default-initialized] value "without checking whether the option is 
> enabled()" is a bug -- the corresponding "trick" never fully worked:
> 
> ```C++
> // TODO: Some callers use .value without checking whether the option is
> // enabled(), accessing the (default-initialized or customized) default
> // value that way. This trick will stop working if we add valued options
> // that can be disabled (e.g., --with-foo=x --without-foo).
> ```
> 
> 
>> However, this is not the case. The helper's response is split into 
>> tokens using a comma as the default delimiter. As a result, only ACLs 
>> like the following will match:
>>      acl staff note group lawyers
>>
>> This behavior occurs because in Acl::NoteCheck::matchNotes(), a comma 
>> is passed as the default delimiter to the expandListEntries() function
> 
> Agreed.
> 

Nod.

> 
>> I would like to propose two changes:
>> 1. Removing the default comma delimiter.
> 
> ... and check enabled() before using the stored value, fixing the bug 
> introduced in 2017 commit 4eac3407.
> 

IMO that is the initial bug causing issues.



> 
>> I am prepared to submit a simple PR to exclude this comma to fix the 
>> incorrect matching of strings containing commas.
>> However, I realize this might be a breaking change for users who 
>> currently rely on this implicit splitting behavior.
> 
> Yes. We should disclose the bug fix in Squid release notes.
> 
> We can also add code to warn admins (via a cache.log WARNING message) 
> when a "note" ACL configured without "-m" looks at an annotation value 
> containing a comma, but that requires more work.
> 
> 
>> 2. Supporting custom delimiters in helper responses.
>> I also propose a PR to support a format where tag values can be passed 
>> as a list with a custom delimiter:
>>      <key>=<delimiter>"<value1><delimiter><value2>..."
>> For example:
>>      group=,"group1,group2,group3"
>>      clt_con_tag=;"tag1;tag2;tag3"
>> In this PR, the helper response would be tokenized based on the 
>> specified custom delimiter, while still supporting delimiter escaping 
>> with a backslash (\).
> 
> I do not think this hack will work well as is, without syntax 
> modifications because Squid already uses double quotes specially in this 
> context. Overloading quotation meaning would be confusing/wrong.
> 
> Overall, I am not excited about this hack, but let's start with these 
> questions about its scope:
> 
> * Can the same effect be achieved today by sending a helper response 
> containing multiple same-name annotations? For example:
> 
>      group=group1 group=group2 group=group3
> 

No. That will be added as three different kv-pair by the helper logic.

The annotations storing kv-pair in MiME syntax with mixed arbitrary 
key=value and key=list,of,values is a problem.


> * If the "note" ACL bug is fixed, do we still need to allow helper to 
> use custom value delimiters?
> 
> * What would Squid do today if it receives a `group=,"a,b"` annotation 
> from a helper? AFAICT from looking at 
> Helper::Reply::parseResponseKeys(), Squid would silently treat the 
> leading comma delimiter as the first character of the received 
> annotation value and keep double quotes, right?

No. All double-quotes are removed.

That input would reach the annotations storage as:
   { key: 'group', value: ',a,b' }

Then the annotation storage saves that as key=value,list syntax and 
expands it later. Same problems all over again.


> 
> * Squid does not treat backslashes in annotation values specially today, 
> does it? If present, they become part of the annotation key or value, 
> right?

No. They are translated by the helper response parser into octets.


Input ' group="a\,b" ' would reach the annotations storage as:
   { key: 'group', value: 'a,b' }


The bug is not in the helper protocol. It does **nothing** with comma. 
Correctly so IMO.


HTH
Amos