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

[Alex Rousskov]

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.

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

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


> 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? Did our helper response parser ever split such fields in the past?

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.


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


> 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

* 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?

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



HTH,

Alex.