Re: Issue with acl note (without -m) splitting helper tokens containing commas
Andrey K <[email protected]> Tue, 21 Apr 2026 13:24:09 +0300
| Newsgroups | gmane.comp.web.squid.devel |
|---|---|
| Message-ID | <CADJd0Y1UHHjFT7NqBemRjb7K4z_=KAXAfUCY9mC9ecy4ofr23g@mail.gmail.com> |
--===============4712027633489750829==
Content-Type: multipart/alternative; boundary="00000000000095baaf064ff5d1a5"
--00000000000095baaf064ff5d1a5
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Hello, Amos and Alex,
Thank you very much for your comments.
> The bug is not in the helper protocol. It does **nothing** with comma.
> Correctly so IMO.
Agreed.
> >> 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 agree with Alex that checking enabled() is the correct way to fix the bug
introduced in commit 4eac3407.
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.
So, following Alex's suggestion, the fix would be:
bool
Acl::NoteCheck::matchNotes(const NotePairs *note) const
{
const CharacterSet *sep =3D delimiters.enabled() ? &delimiters.value :
nullptr;
const NotePairs::Entries &entries =3D note->expandListEntries(sep);
for (auto e: entries)
if (data->match(e.getRaw()))
return true;
return false;
}
May I submit the PR with this change?
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.
Kind regards,
Ankor.
=D0=B2=D1=82, 21 =D0=B0=D0=BF=D1=80. 2026=E2=80=AF=D0=B3. =D0=B2 11:02, Amo=
s Jeffries <[email protected]>:
> On 21/04/2026 06:59, Alex Rousskov wrote:
> > On 2026-04-17 09:08, Andrey K wrote:
> >
> >> While working with annotations, I=E2=80=99ve 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=3D"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=3D"Staff:accountants,lawyers,security"
> >
> > The above implies that helper response parser should not split group=3D=
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=3Dvalue-list, only key=3Dvalue (singular value).
>
>
>
> > BTW, 2015 commit 76ee67ac used a very similar example. AFAICT by lookin=
g
> > 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 option=
s
> > // that can be disabled (e.g., --with-foo=3Dx --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>=3D<delimiter>"<value1><delimiter><value2>..."
> >> For example:
> >> group=3D,"group1,group2,group3"
> >> clt_con_tag=3D;"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 thi=
s
> > 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=3Dgroup1 group=3Dgroup2 group=3Dgroup3
> >
>
> 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=3Dvalue and key=3Dlist,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=3D,"a,b"` annotatio=
n
> > 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=3Dvalue,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=3D"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
> [email protected]
> https://lists.squid-cache.org/listinfo/squid-dev
>
--00000000000095baaf064ff5d1a5
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr"><div dir=3D"ltr">Hello, Amos and Alex,<div><br></div><div>=
Thank you very much for your comments.</div><div><br></div><div>> The bu=
g is not in the helper protocol. It does **nothing** with comma.<br>> Co=
rrectly 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 i=
s the initial bug causing issues.<div><br></div><div>I agree with Alex that=
checking <font face=3D"monospace">enabled()</font> is the correct way to f=
ix 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 th=
at a comma should be used as a separator when the <font face=3D"monospace">=
-m</font> option is specified without parameters.</div><div><br>So, followi=
ng Alex's suggestion, the fix would be:</div><div><br></div><div><font =
face=3D"monospace">bool<br>Acl::NoteCheck::matchNotes(const NotePairs *note=
) const<br>{<br>=C2=A0 =C2=A0 const CharacterSet *sep =3D delimiters.enable=
d() ? &delimiters.value : nullptr;<br>=C2=A0 =C2=A0 const NotePairs::En=
tries &entries =3D note->expandListEntries(sep);<br>=C2=A0 =C2=A0 fo=
r (auto e: entries)<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (data->match(e.get=
Raw()))<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return true;<br>=C2=A0=
=C2=A0 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" AC=
L configured without "-m" looks at an annotation value<br>> &g=
t; containing a comma, but that requires more work.</div><div><br></div><di=
v>I believe this warning might be useful for the current buggy version.=C2=
=A0</div><div>However, with this fix, the behavior will correctly follow th=
e documentation, so a warning shouldn't be necessary in the patched ver=
sion.</div><div><br></div><div><br>Kind regards,</div></div><div>=C2=A0 =C2=
=A0 Ankor.</div><div><br></div><br><div class=3D"gmail_quote gmail_quote_co=
ntainer"><div dir=3D"ltr" class=3D"gmail_attr">=D0=B2=D1=82, 21 =D0=B0=D0=
=BF=D1=80. 2026=E2=80=AF=D0=B3. =D0=B2 11:02, Amos Jeffries <<a href=3D"=
mailto:[email protected]">[email protected]</a>>:<br></div><blockq=
uote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1p=
x solid rgb(204,204,204);padding-left:1ex">On 21/04/2026 06:59, Alex Roussk=
ov wrote:<br>
> On 2026-04-17 09:08, Andrey K wrote:<br>
> <br>
>> While working with annotations, I=E2=80=99ve noticed an inconsiste=
ncy in how <br>
>> acl note (without the -m option) handles tokens received from help=
ers <br>
>> when they contain a comma.<br>
> <br>
> I think it is important to note that there are several distinct player=
s <br>
> here, including:<br>
> <br>
> 1. What annotations the helper sends to Squid.<br>
> <br>
<br>
Which is:<br>
<br>
=C2=A0 =C2=A0 =C2=A0group=3D"Staff:accountants,lawyers,security"<=
br>
<br>
<br>
> 2. How helper response parser converts received helper response into<b=
r>
>=C2=A0 =C2=A0=C2=A0 transaction annotations.<br>
> <br>
<br>
Which is:<br>
<br>
=C2=A0 =C2=A01) remove double-quotes<br>
=C2=A0 =C2=A02) translate \-escapes within quoted-string values<br>
=C2=A0 =C2=A03) 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 annotation=
s.<br>
>=C2=A0 =C2=A0=C2=A0 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>
=C2=A0 (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>
>> =C2=A0=C2=A0 =C2=A0 acl staff note group Staff:accountants,lawyers=
,security<br>
>> should match a helper response such as:<br>
>> =C2=A0=C2=A0 =C2=A0 group=3D"Staff:accountants,lawyers,securi=
ty"<br>
> <br>
> The above implies that helper response parser should not split group=
=3DX <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=3Dvalue-list, only key=3Dvalue (singular value).<br>
<br>
<br>
<br>
> BTW, 2015 commit 76ee67ac used a very similar example. AFAICT by looki=
ng <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). Th=
e 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 i=
s<br>
> // enabled(), accessing the (default-initialized or customized) defaul=
t<br>
> // value that way. This trick will stop working if we add valued optio=
ns<br>
> // that can be disabled (e.g., --with-foo=3Dx --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 A=
CLs <br>
>> like the following will match:<br>
>> =C2=A0=C2=A0 =C2=A0 acl staff note group lawyers<br>
>><br>
>> This behavior occurs because in Acl::NoteCheck::matchNotes(), a co=
mma <br>
>> is passed as the default delimiter to the expandListEntries() func=
tion<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 t=
he <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 pa=
ssed <br>
>> as a list with a custom delimiter:<br>
>> =C2=A0=C2=A0 =C2=A0 <key>=3D<delimiter>"<value=
1><delimiter><value2>..."<br>
>> For example:<br>
>> =C2=A0=C2=A0 =C2=A0 group=3D,"group1,group2,group3"<br>
>> =C2=A0=C2=A0 =C2=A0 clt_con_tag=3D;"tag1;tag2;tag3"<br>
>> In this PR, the helper response would be tokenized based on the <b=
r>
>> specified custom delimiter, while still supporting delimiter escap=
ing <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 th=
is <br>
> context. Overloading quotation meaning would be confusing/wrong.<br>
> <br>
> Overall, I am not excited about this hack, but let's start with th=
ese <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>
>=C2=A0 =C2=A0=C2=A0=C2=A0 group=3Dgroup1 group=3Dgroup2 group=3Dgroup3<=
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=3Dvalue and key=3Dlist,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=3D,"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>
=C2=A0 =C2=A0{ key: 'group', value: ',a,b' }<br>
<br>
Then the annotation storage saves that as key=3Dvalue,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 toda=
y, <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=3D"a\,b" ' would reach the annotations stor=
age as:<br>
=C2=A0 =C2=A0{ 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=3D"mailto:[email protected]" target=3D"_blank">squid-=
[email protected]</a><br>
<a href=3D"https://lists.squid-cache.org/listinfo/squid-dev" rel=3D"norefer=
rer" target=3D"_blank">https://lists.squid-cache.org/listinfo/squid-dev</a>=
<br>
</blockquote></div></div>
--00000000000095baaf064ff5d1a5--
--===============4712027633489750829==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
_______________________________________________
squid-dev mailing list
[email protected]
https://lists.squid-cache.org/listinfo/squid-dev
--===============4712027633489750829==--