<div dir="ltr"><div dir="ltr">Hello, Amos and Alex,<div><br></div><div>Thank you very much for your comments.</div><div><br></div><div>> The bug is not in the helper protocol. It does **nothing** with comma.<br>> Correctly so IMO.</div><div>Agreed.</div><div><br></div><div>> >> 1. Removing the default comma delimiter.</div>> ><br>> > ... and check enabled() before using the stored value, fixing the bug<br>> > introduced in 2017 commit 4eac3407.<br>> ><br>><br>> IMO that is the initial bug causing issues.<div><br></div><div>I agree with Alex that checking <font face="monospace">enabled()</font> is the correct way to fix the bug introduced in commit 4eac3407.</div><div><br>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 <font face="monospace">-m</font> option is specified without parameters.</div><div><br>So, following Alex's suggestion, the fix would be:</div><div><br></div><div><font face="monospace">bool<br>Acl::NoteCheck::matchNotes(const NotePairs *note) const<br>{<br> const CharacterSet *sep = delimiters.enabled() ? &delimiters.value : nullptr;<br> const NotePairs::Entries &entries = note->expandListEntries(sep);<br> for (auto e: entries)<br> if (data->match(e.getRaw()))<br> return true;<br> return false;<br>}</font><br></div><div><br></div><div>May I submit the PR with this change?</div><div><br>Regarding the warning for admins:</div><div><br></div><div>> > We can also add code to warn admins (via a cache.log WARNING message)</div><div>> > when a "note" ACL configured without "-m" looks at an annotation value<br>> > containing a comma, but that requires more work.</div><div><br></div><div>I believe this warning might be useful for the current buggy version. </div><div>However, with this fix, the behavior will correctly follow the documentation, so a warning shouldn't be necessary in the patched version.</div><div><br></div><div><br>Kind regards,</div></div><div> Ankor.</div><div><br></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">вт, 21 апр. 2026 г. в 11:02, Amos Jeffries <<a href="mailto:squid3@treenet.co.nz">squid3@treenet.co.nz</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 21/04/2026 06:59, Alex Rousskov wrote:<br> > On 2026-04-17 09:08, Andrey K wrote:<br> > <br> >> While working with annotations, I’ve noticed an inconsistency in how <br> >> acl note (without the -m option) handles tokens received from helpers <br> >> when they contain a comma.<br> > <br> > I think it is important to note that there are several distinct players <br> > here, including:<br> > <br> > 1. What annotations the helper sends to Squid.<br> > <br> <br> Which is:<br> <br> group="Staff:accountants,lawyers,security"<br> <br> <br> > 2. How helper response parser converts received helper response into<br> > transaction annotations.<br> > <br> <br> Which is:<br> <br> 1) remove double-quotes<br> 2) translate \-escapes within quoted-string values<br> 3) translate %-coded within token values<br> <br> Nothing more. ',' is not special for that parser.<br> <br> <br> > 3. How the "note" ACL code interprets transaction annotations.<br> > These annotations may come from sources other than a helper.<br> > <br> <br> This is where ',' becomes special as a list delimiter.<br> <br> The annotation system interprets "a,b,c" as a set of three values, <br> stored as a list.<br> <br> (I do not understand why we need to store annotations in their <br> serialized format in the first place. It is generally a bad design.)<br> <br> <br> > <br> >> According to the documentation, an ACL like this:<br> >> acl staff note group Staff:accountants,lawyers,security<br> >> should match a helper response such as:<br> >> group="Staff:accountants,lawyers,security"<br> > <br> > The above implies that helper response parser should not split group=X <br> > response fields (using comma as a delimiter). Do we document that <br> > anywhere?<br> <br> The only thing the helper response parser does is remove the DQUOTE <br> characters around the value string.<br> <br> As you noted below, the problem is ACL logic interaction with the <br> annotation storage.<br> <br> <br> > Did our helper response parser ever split such fields in the <br> > past?<br> > <br> <br> If it did that was a bug. Prior to the annotations feature we did not <br> support key=value-list, only key=value (singular value).<br> <br> <br> <br> > BTW, 2015 commit 76ee67ac used a very similar example. AFAICT by looking <br> > at that code, we did not apply value delimiters by default back then <br> > (i.e. when ACL_F_SUBSTRING a.k.a. "-m" flag was not set). The bug was <br> > introduced in 2017 commit 4eac3407 that replaced a possibly-nil <br> > `flags.delimiters()` with a never-nil `&delimiters.value`.<br> > <br> > The following comment suggests that we missed the fact that using the <br> > [default-initialized] value "without checking whether the option is <br> > enabled()" is a bug -- the corresponding "trick" never fully worked:<br> > <br> > ```C++<br> > // TODO: Some callers use .value without checking whether the option is<br> > // enabled(), accessing the (default-initialized or customized) default<br> > // value that way. This trick will stop working if we add valued options<br> > // that can be disabled (e.g., --with-foo=x --without-foo).<br> > ```<br> > <br> > <br> >> However, this is not the case. The helper's response is split into <br> >> tokens using a comma as the default delimiter. As a result, only ACLs <br> >> like the following will match:<br> >> acl staff note group lawyers<br> >><br> >> This behavior occurs because in Acl::NoteCheck::matchNotes(), a comma <br> >> is passed as the default delimiter to the expandListEntries() function<br> > <br> > Agreed.<br> > <br> <br> Nod.<br> <br> > <br> >> I would like to propose two changes:<br> >> 1. Removing the default comma delimiter.<br> > <br> > ... and check enabled() before using the stored value, fixing the bug <br> > introduced in 2017 commit 4eac3407.<br> > <br> <br> IMO that is the initial bug causing issues.<br> <br> <br> <br> > <br> >> I am prepared to submit a simple PR to exclude this comma to fix the <br> >> incorrect matching of strings containing commas.<br> >> However, I realize this might be a breaking change for users who <br> >> currently rely on this implicit splitting behavior.<br> > <br> > Yes. We should disclose the bug fix in Squid release notes.<br> > <br> > We can also add code to warn admins (via a cache.log WARNING message) <br> > when a "note" ACL configured without "-m" looks at an annotation value <br> > containing a comma, but that requires more work.<br> > <br> > <br> >> 2. Supporting custom delimiters in helper responses.<br> >> I also propose a PR to support a format where tag values can be passed <br> >> as a list with a custom delimiter:<br> >> <key>=<delimiter>"<value1><delimiter><value2>..."<br> >> For example:<br> >> group=,"group1,group2,group3"<br> >> clt_con_tag=;"tag1;tag2;tag3"<br> >> In this PR, the helper response would be tokenized based on the <br> >> specified custom delimiter, while still supporting delimiter escaping <br> >> with a backslash (\).<br> > <br> > I do not think this hack will work well as is, without syntax <br> > modifications because Squid already uses double quotes specially in this <br> > context. Overloading quotation meaning would be confusing/wrong.<br> > <br> > Overall, I am not excited about this hack, but let's start with these <br> > questions about its scope:<br> > <br> > * Can the same effect be achieved today by sending a helper response <br> > containing multiple same-name annotations? For example:<br> > <br> > group=group1 group=group2 group=group3<br> > <br> <br> No. That will be added as three different kv-pair by the helper logic.<br> <br> The annotations storing kv-pair in MiME syntax with mixed arbitrary <br> key=value and key=list,of,values is a problem.<br> <br> <br> > * If the "note" ACL bug is fixed, do we still need to allow helper to <br> > use custom value delimiters?<br> > <br> > * What would Squid do today if it receives a `group=,"a,b"` annotation <br> > from a helper? AFAICT from looking at <br> > Helper::Reply::parseResponseKeys(), Squid would silently treat the <br> > leading comma delimiter as the first character of the received <br> > annotation value and keep double quotes, right?<br> <br> No. All double-quotes are removed.<br> <br> That input would reach the annotations storage as:<br> { key: 'group', value: ',a,b' }<br> <br> Then the annotation storage saves that as key=value,list syntax and <br> expands it later. Same problems all over again.<br> <br> <br> > <br> > * Squid does not treat backslashes in annotation values specially today, <br> > does it? If present, they become part of the annotation key or value, <br> > right?<br> <br> No. They are translated by the helper response parser into octets.<br> <br> <br> Input ' group="a\,b" ' would reach the annotations storage as:<br> { key: 'group', value: 'a,b' }<br> <br> <br> The bug is not in the helper protocol. It does **nothing** with comma. <br> Correctly so IMO.<br> <br> <br> HTH<br> Amos<br> <br> _______________________________________________<br> squid-dev mailing list<br> <a href="mailto:squid-dev@lists.squid-cache.org" target="_blank">squid-dev@lists.squid-cache.org</a><br> <a href="https://lists.squid-cache.org/listinfo/squid-dev" rel="noreferrer" target="_blank">https://lists.squid-cache.org/listinfo/squid-dev</a><br> </blockquote></div></div>