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

[Andrey K]

Hello,

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=,.
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.
Alternatively, to avoid breaking existing installations, we could keep the
comma as the default but allow users to specify an empty delimiter to
correctly handle tokens that may contain commas and to ensure Squid's
behavior aligns with the documentation.

Which of these approaches would you prefer me to implement in the PR?

Kind regards,
    Ankor.

пт, 17 апр. 2026 г. в 16:08, Andrey K <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.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20260420/b4759344/attachment.htm>