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

[Alex Rousskov]

On 2026-04-21 06:24, Andrey K wrote:
>  > >> 1. Removing the default comma delimiter.
>  > >
>  > > ... and check enabled() before using the stored value, fixing the bug
>  > > introduced in 2017 commit 4eac3407.
> 
> I agree with Alex that checking enabled() is the correct way to fix the 
> bug introduced in commit 4eac3407.

Glad we are on the same page.


> I have reconsidered my previous idea about removing the default comma 
> delimiter from the class constructor. I now see that it should stay, as 
> the documentation states that a comma should be used as a separator when 
> the -m option is specified without parameters.

Yes, probably, at least for now. I am pretty sure that an ideal 
implementation would handle that default separator differently because 
the default is not "delimited by commas"; the default is "not 
delimited". Ideally, Squid should not do unnecessary work... However, 
your future PR probably should _not_ be responsible for reaching that 
ideal because improving how we handle such default values probably 
requires significant refactoring of an already complex code.


> So, following Alex's suggestion, the fix would be:
> 
> bool
> Acl::NoteCheck::matchNotes(const NotePairs *note) const
> {
>      const CharacterSet *sep = delimiters.enabled() ? &delimiters.value 
> : nullptr;
>      const NotePairs::Entries &entries = note->expandListEntries(sep);
>      for (auto e: entries)
>          if (data->match(e.getRaw()))
>              return true;
>      return false;
> }
> 
> May I submit the PR with this change?

Yes, of course! However, the above is not enough to fully address this 
bug AFAICT. My suggestion below should expose the cases missed by the 
above fix.


Please try to refactor so that accessing `delimiters.value` becomes 
impossible when delimiters are not enabled. Conceptually, 
ACLFlags::delimiters() removed in 2017 commit 4eac3407 got it right. We 
should restore those protections.

I have not checked all the details, but I would _start_ by replacing the 
following OptionValue data member:

     Value value; ///< final value storage, possibly after conversions

with:

     /// option value (if the option was enabled) or nil (otherwise)
     const Value *value() const { return enabled() ? 
&explicitOrDefaultValue : nullptr; }

     // TODO: Make private.
     /// Final value storage, possibly after conversions.
     /// When `valued` is true, this is an explicitly set value.
     /// Otherwise, this is the default value.
     Value explicitOrDefaultValue;


Ideally, explicitOrDefaultValue should be private, but achieving that 
ideal requires more refactoring work, and I do not recommend it at this 
time.

Current OptionValue::value users should be adjusted accordingly, of 
course, and I hope that the compiler will catch most cases that a manual 
search has missed. The "TODO: Some callers..." comment will need to be 
adjusted as well.


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


HTH,

Alex.


> вт, 21 апр. 2026 г. в 11:02, Amos Jeffries <squid3 at treenet.co.nz 
> <mailto:squid3 at treenet.co.nz>>:
> 
>     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
> 
>     _______________________________________________
>     squid-dev mailing list
>     squid-dev at lists.squid-cache.org <mailto:squid-dev at lists.squid-cache.org>
>     https://lists.squid-cache.org/listinfo/squid-dev
>     <https://lists.squid-cache.org/listinfo/squid-dev>
> 
> 
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> https://lists.squid-cache.org/listinfo/squid-dev