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

[Alex Rousskov]

On 2026-04-20 05:50, Andrey K wrote:

> I think I’ve come up with a way to minimize the impact of removing the 
> default comma delimiter on existing Squid setups.
> We can introduce a new build-time configuration option, 
> --with-annotation-values-default-delimiter, so that users who rely on 
> the comma separator can rebuild Squid with 
> --with-annotation-values-default-delimiter=,.

Please do not introduce ./configure options for functionality that can 
be configured by more dynamic means (e.g., via squid.conf).


> While this approach is more conceptually sound and aligns better with 
> the documentation, I am concerned that many users rely on 
> vendor-provided binary packages and won't have the opportunity to 
> recompile Squid from source.

Your concern is valid, and there are other reasons we should avoid 
avoidable ./configure options.

I will respond to your other suggestions separately.


Thank you,

Alex.


> пт, 17 апр. 2026 г. в 16:08, Andrey K <ankor2023 at gmail.com 
> <mailto:ankor2023 at gmail.com>>:
> 
>     Hello,
> 
>     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.
> 
>     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"
> 
>     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:
>          bool
>          ACLNoteStrategy::matchNotes(ACLData<MatchType> *noteData, const
>     NotePairs *note) const
>          {
>              const NotePairs::Entries &entries =
>     note->expandListEntries(&delimiters.value);
>              for (auto e: entries)
>                  if (noteData->match(e.getRaw()))
>                      return true;
>              return false;
>          }
> 
>     After reviewing the source code, I found that the comma is added in
>     src/acl/Note.h within the AnnotationCheck class constructor:
>          AnnotationCheck(): delimiters(CharacterSet("__FILE__", ",")) {}
> 
>     Using a comma as the default delimiter makes it difficult to
>     validate full tokens that may naturally contain commas.
> 
>     I would like to propose two changes:
>     1. Removing the default comma delimiter.
>     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.
> 
>     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 (\).
>     In this scenario, having a hardcoded comma as the default delimiter
>     in the AnnotationCheck class would also create complications.
> 
>     I would appreciate your thoughts on these proposals and whether they
>     align with the project's roadmap.
> 
>     Kind regards,
>          Ankor.
> 
> 
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> https://lists.squid-cache.org/listinfo/squid-dev