Re: Issue with acl note (without -m) splitting helper tokens containing commas
Andrey K <[email protected]> Mon, 20 Apr 2026 12:50:21 +0300
| Newsgroups | gmane.comp.web.squid.devel |
|---|---|
| Message-ID | <CADJd0Y2eRvv9dtZKRFgzQk3bRZJCXvzxkYCAzSgqw-fweGd7MA@mail.gmail.com> |
--===============3927335443190027876==
Content-Type: multipart/alternative; boundary="000000000000d9d649064fe13a6f"
--000000000000d9d649064fe13a6f
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Hello,
I think I=E2=80=99ve 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=3D,.
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.
=D0=BF=D1=82, 17 =D0=B0=D0=BF=D1=80. 2026=E2=80=AF=D0=B3. =D0=B2 16:08, And=
rey K <[email protected]>:
> Hello,
>
> 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.
>
> 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"
>
> 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 =3D
> 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 ful=
l
> 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 currentl=
y
> 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>=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 (\).
> In this scenario, having a hardcoded comma as the default delimiter in th=
e AnnotationCheck
> class would also create complications.
>
> I would appreciate your thoughts on these proposals and whether they alig=
n
> with the project's roadmap.
>
> Kind regards,
> Ankor.
>
>
--000000000000d9d649064fe13a6f
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr">Hello,<div><br></div><div>I think I=E2=80=99ve come up wit=
h a way to minimize the impact of removing the default comma delimiter on e=
xisting Squid setups.</div><div>We can introduce a new build-time configura=
tion option, <font face=3D"monospace">--with-annotation-values-default-deli=
miter</font>, so that users who rely on the comma separator can rebuild Squ=
id with <font face=3D"monospace">--with-annotation-values-default-delimiter=
=3D,</font>.=C2=A0</div><div>While this approach is more conceptually sound=
and aligns better with the documentation, I am concerned that many users r=
ely on vendor-provided binary packages and won't have the opportunity t=
o recompile Squid from source.=C2=A0</div><div>Alternatively, to avoid brea=
king existing installations, we could keep the <font face=3D"monospace">com=
ma</font> as the default but allow users to specify an empty delimiter to c=
orrectly handle tokens that may contain commas and to ensure Squid's be=
havior aligns with the documentation.=C2=A0</div><div><br></div><div>Which =
of these approaches would you prefer me to implement in the PR?</div><div><=
br></div><div>Kind regards,</div><div>=C2=A0 =C2=A0 Ankor.</div></div><br><=
div class=3D"gmail_quote gmail_quote_container"><div dir=3D"ltr" class=3D"g=
mail_attr">=D0=BF=D1=82, 17 =D0=B0=D0=BF=D1=80. 2026=E2=80=AF=D0=B3. =D0=B2=
16:08, Andrey K <<a href=3D"mailto:[email protected]">ankor2023@gmail=
.com</a>>:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0p=
x 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><d=
iv dir=3D"ltr">Hello,<div><br>While working with annotations, I=E2=80=99ve =
noticed an inconsistency in how <font face=3D"monospace">acl note</font> (w=
ithout the <font face=3D"monospace">-m</font> option) handles tokens receiv=
ed from helpers when they contain a comma.<br><br></div><div>According to t=
he documentation, an ACL like this:<br><font face=3D"monospace">=C2=A0 =C2=
=A0 acl staff note group Staff:accountants,lawyers,security</font><br>shoul=
d match a helper response such as:<br><font face=3D"monospace">=C2=A0 =C2=
=A0 group=3D"Staff:accountants,lawyers,security"</font><br><br><d=
iv>However, this is not the case. The helper's response is split into t=
okens using a comma as the default delimiter. As a result, only ACLs like t=
he following will match:</div><div><font face=3D"monospace">=C2=A0 =C2=A0 a=
cl staff note group lawyers</font><br><br></div><div>This behavior occurs b=
ecause in Acl::NoteCheck::matchNotes(), a comma is passed as the default de=
limiter to the expandListEntries() function:</div><div><font face=3D"monosp=
ace">=C2=A0 =C2=A0 bool<br>=C2=A0 =C2=A0 ACLNoteStrategy::matchNotes(ACLDat=
a<MatchType> *noteData, const NotePairs *note) const<br>=C2=A0 =C2=A0=
{<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 const NotePairs::Entries &entries =3D=
note->expandListEntries(&delimiters.value);<br>=C2=A0 =C2=A0 =C2=A0=
=C2=A0 for (auto e: entries)<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
if (noteData->match(e.getRaw()))<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 return true;<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 return fal=
se;<br>=C2=A0 =C2=A0 }<br></font></div><div><font face=3D"monospace"><br></=
font></div><div>After reviewing the source code, I found that the comma is =
added in src/acl/Note.h within the AnnotationCheck class constructor:<br><f=
ont face=3D"monospace">=C2=A0 =C2=A0 AnnotationCheck(): delimiters(Characte=
rSet("__FILE__", ",")) {}</font><br><br></div><div>Usin=
g a comma as the default delimiter makes it difficult to validate full toke=
ns that may naturally contain commas.<br><br></div><div>I would like to pro=
pose two changes:<br>1. Removing the default comma delimiter.</div><div>I a=
m prepared to submit a simple PR to exclude this comma to fix the incorrect=
matching of strings containing commas.=C2=A0</div><div>However, I realize =
this might be a breaking change for users who currently rely on this implic=
it splitting behavior.</div><div><br>2. Supporting custom delimiters in hel=
per responses.</div><div>I also propose a PR to support a format where tag =
values can be passed as a list with a custom delimiter:<br><font face=3D"mo=
nospace">=C2=A0 =C2=A0 <key>=3D<delimiter>"<value1>&=
lt;delimiter><value2>..."</font><br>For example:<br><font fac=
e=3D"monospace">=C2=A0 =C2=A0 group=3D,"group1,group2,group3"<br>=
=C2=A0 =C2=A0 clt_con_tag=3D;"tag1;tag2;tag3"</font><br>In this P=
R, the helper response would be tokenized based on the specified custom del=
imiter, while still supporting delimiter escaping with a backslash (<font f=
ace=3D"monospace">\</font>).<br>In this scenario, having a hardcoded comma =
as the default delimiter in the <font face=3D"monospace">AnnotationCheck </=
font>class would also create complications.</div><div><br>I would appreciat=
e your thoughts on these proposals and whether they align with the project&=
#39;s roadmap.</div><div><br></div><div>Kind regards,</div><div>=C2=A0 =C2=
=A0 Ankor.</div><div><br></div></div></div>
</blockquote></div>
--000000000000d9d649064fe13a6f--
--===============3927335443190027876==
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
--===============3927335443190027876==--