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

[Andrey K]

Hello Alex and Amos,

I would like to follow up on our previous discussion regarding this issue.

> > Regarding the warning for admins:
>   >
>   >  > > 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.
>   >
>   > I believe this warning might be useful for the current buggy version.
>   > However, with this fix, the behavior will correctly follow the
>   > documentation, so a warning shouldn't be necessary in the patched
version.
>
>   That warning would be useful in several cases. For example, it would be
>   useful for an admin of a used-to-work-before-the-upgrade-Squid who does
>   not realize that their helper is returning a list of values and that
>   they now must start using "-m" for their upgraded Squid to (continue to)
>   handle that case correctly.
>
>   AFAICT, the biggest problem with that warning is that admins of helpers
>   that return values with commas that should _not_ use "-m" (because those
>   commas are not used as sub-value separators) would have to manually
>   disable the warning (via cache_log_message or, if we do even more extra
>   work, via something like "+m" in individual ACL definitions).


I have prepared a new PR that implements warnings when the '-m' flag is
missing from the note ACL while the helper's response contains commas. It
also introduces the '+m' flag to explicitly disable splitting
comma-separated annotation values and suppress these warnings.

May I proceed with publishing it on GitHub?

I suggest backporting this patch to Squid 7.x and including it in a few
upcoming Squid 8.x releases. After a few cycles, we can remove these
warnings from the codebase. Since they are intended to be short-lived
across only a few versions, I also suggest omitting them from
doc/debug-messages.dox.

Additionally, the '+m' option should probably remain undocumented, as it
will become redundant once the bugfix in PR #2410 is fully applied
(assuming it will be included in Squid 8 releases).

Kind regards,
    Ankor.

ср, 22 апр. 2026 г. в 12:46, Andrey K <ankor2023 at gmail.com>:

> Hello, Alex and Amos,
>
> I have submitted the PR: https://github.com/squid-cache/squid/pull/2410
>
> Alex, thank you very much for your recommendations regarding the code
> refactoring.
>
> > Ideally, explicitOrDefaultValue should be private, but achieving that
> > ideal requires more refactoring work, and I do not recommend it at this
> > time.
>
> I didn't see any difficulties there, so I've already made
> explicitOrDefaultValue private.
>
>
> > The "TODO: Some callers..." comment will need to be
> > adjusted as well.
>
> I have removed the "TODO" comment entirely since access to the storage
> variable is now properly protected via accessors.
>
> Kind regards,
>     Ankor.
>
> вт, 21 апр. 2026 г. в 17:47, Alex Rousskov <
> rousskov at measurement-factory.com>:
>
>> On 2026-04-21 04:01, Amos Jeffries wrote:
>> > I do not understand why we need to store annotations in their
>> > serialized format in the first place.
>>
>> Today, we store "serialized" annotations because different ACLs may
>> specify different delimiters for the same imported annotation value.
>> There are two reasonable ways to look at the current design:
>>
>> A: This design is broken: Deserialization should happen at annotation
>> import time (e.g., in helper response parser). Something like "-m"
>> belongs to helper configuration, not ACLs. 2015 commit 76ee67ac was too
>> focused on its "_match_ substrings of _existing_ annotations" use cases
>> to realize that this is an import feature, not a matching feature.
>>
>> B: This design is correct: Future ACLs may specify match regexes or even
>> custom matching functions that would interpret imported values
>> (effectively deserializign them) differently in different contexts. For
>> example, a helper may send "metadata_" annotation for a denied user
>> using a different syntax than the one used for "metadata_" annotation
>> for an allowed user. 2015 commit 76ee67ac got it right: We should place
>> "-m" inside ACLs, just like we place "-i" inside ACLs. If helpers want
>> Squid to import a list of values, we should add such support, but that
>> is a separate matter.
>>
>>
>> >>> 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.
>>
>> If the above does add three different name=value annotations with the
>> same name, then the answer to my question is "Yes" rather than "No".
>>
>> I did _not_ test this, but, after looking at
>> Helper::Reply::parseResponseKeys() code, I suspect that, yes, "the same
>> effect be achieved today by sending a helper response containing
>> multiple same-name annotations". If that untested theory is correct,
>> then we do not need an additional hack to allow helpers to send Squid a
>> list of same-name values.
>>
>> Alex.
>>
>> _______________________________________________
>> squid-dev mailing list
>> squid-dev at lists.squid-cache.org
>> https://lists.squid-cache.org/listinfo/squid-dev
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20260529/a1898e17/attachment.htm>