Issue with acl note (without -m) splitting helper tokens containing commas

--===============1976334727149481915==
Content-Type: multipart/alternative; boundary="0000000000000933a8064fa7a776"

--0000000000000933a8064fa7a776
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hello,

While working with annotations, I=E2=80=99ve noticed an inconsistency in ho=
w 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 full
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 currently
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
the AnnotationCheck
class would also create complications.

I would appreciate your thoughts on these proposals and whether they align
with the project's roadmap.

Kind regards,
    Ankor.

--0000000000000933a8064fa7a776
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Hello,<div><br>While working with annotations, I=E2=80=99v=
e noticed an inconsistency in how <font face=3D"monospace">acl note</font> =
(without the <font face=3D"monospace">-m</font> option) handles tokens rece=
ived from helpers when they contain a comma.<br><br></div><div>According to=
 the documentation, an ACL like this:<br><font face=3D"monospace">=C2=A0 =
=C2=A0 acl staff note group Staff:accountants,lawyers,security</font><br>sh=
ould match a helper response such as:<br><font face=3D"monospace">=C2=A0 =
=C2=A0 group=3D&quot;Staff:accountants,lawyers,security&quot;</font><br><br=
><div>However, this is not the case. The helper&#39;s response is split int=
o tokens using a comma as the default delimiter. As a result, only ACLs lik=
e the following will match:</div><div><font face=3D"monospace">=C2=A0 =C2=
=A0 acl staff note group lawyers</font><br><br></div><div>This behavior occ=
urs because in Acl::NoteCheck::matchNotes(), a comma is passed as the defau=
lt delimiter to the expandListEntries() function:</div><div><font face=3D"m=
onospace">=C2=A0 =C2=A0 bool<br>=C2=A0 =C2=A0 ACLNoteStrategy::matchNotes(A=
CLData&lt;MatchType&gt; *noteData, const NotePairs *note) const<br>=C2=A0 =
=C2=A0 {<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 const NotePairs::Entries &amp;entri=
es =3D note-&gt;expandListEntries(&amp;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-&gt;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 ret=
urn false;<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 co=
mma is added in src/acl/Note.h within the AnnotationCheck class constructor=
:<br><font face=3D"monospace">=C2=A0 =C2=A0 AnnotationCheck(): delimiters(C=
haracterSet(&quot;__FILE__&quot;, &quot;,&quot;)) {}</font><br><br></div><d=
iv>Using a comma as the default delimiter makes it difficult to validate fu=
ll tokens that may naturally contain commas.<br><br></div><div>I would like=
 to propose two changes:<br>1. Removing the default comma delimiter.</div><=
div>I am prepared to submit a simple PR to exclude this comma to fix the in=
correct matching of strings containing commas.=C2=A0</div><div>However, I r=
ealize this might be a breaking change for users who currently rely on this=
 implicit splitting behavior.</div><div><br>2. Supporting custom delimiters=
 in helper responses.</div><div>I also propose a PR to support a format whe=
re tag values can be passed as a list with a custom delimiter:<br><font fac=
e=3D"monospace">=C2=A0 =C2=A0 &lt;key&gt;=3D&lt;delimiter&gt;&quot;&lt;valu=
e1&gt;&lt;delimiter&gt;&lt;value2&gt;...&quot;</font><br>For example:<br><f=
ont face=3D"monospace">=C2=A0 =C2=A0 group=3D,&quot;group1,group2,group3&qu=
ot;<br>=C2=A0 =C2=A0 clt_con_tag=3D;&quot;tag1;tag2;tag3&quot;</font><br>In=
 this PR, the helper response would be tokenized based on the specified cus=
tom delimiter, while still supporting delimiter escaping with a backslash (=
<font face=3D"monospace">\</font>).<br>In this scenario, having a hardcoded=
 comma as the default delimiter in the <font face=3D"monospace">AnnotationC=
heck </font>class would also create complications.</div><div><br>I would ap=
preciate your thoughts on these proposals and whether they align with the p=
roject&#39;s roadmap.</div><div><br></div><div>Kind regards,</div><div>=C2=
=A0 =C2=A0 Ankor.</div><div><br></div></div></div>

--0000000000000933a8064fa7a776--

--===============1976334727149481915==
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

--===============1976334727149481915==--