Re: Issue with acl note (without -m) splitting helper tokens containing commas
Andrey K <[email protected]> Fri, 29 May 2026 12:30:54 +0300
| Newsgroups | gmane.comp.web.squid.devel |
|---|---|
| Message-ID | <CADJd0Y2mvpPdV-4=gowGFChZjf=wm2QrzovRBvjy-DxAdKPSmA@mail.gmail.com> |
--===============2306553263455119260==
Content-Type: multipart/alternative; boundary="000000000000084e330652f181bc"
--000000000000084e330652f181bc
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Hello Alex and Amos,
I would like to follow up on our previous discussion regarding this issue.
> > 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 thos=
e
> 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).
I have prepared a new PR that implements warnings when the '-m' flag is
missing from the note ACL while the helper's response contains commas. It
also introduces the '+m' flag to explicitly disable splitting
comma-separated annotation values and suppress these warnings.
May I proceed with publishing it on GitHub?
I suggest backporting this patch to Squid 7.x and including it in a few
upcoming Squid 8.x releases. After a few cycles, we can remove these
warnings from the codebase. Since they are intended to be short-lived
across only a few versions, I also suggest omitting them from
doc/debug-messages.dox.
Additionally, the '+m' option should probably remain undocumented, as it
will become redundant once the bugfix in PR #2410 is fully applied
(assuming it will be included in Squid 8 releases).
Kind regards,
Ankor.
=D1=81=D1=80, 22 =D0=B0=D0=BF=D1=80. 2026=E2=80=AF=D0=B3. =D0=B2 12:46, And=
rey K <[email protected]>:
> Hello, Alex and Amos,
>
> I have submitted the PR: https://github.com/squid-cache/squid/pull/2410
>
> Alex, thank you very much for your recommendations regarding the code
> refactoring.
>
> > Ideally, explicitOrDefaultValue should be private, but achieving that
> > ideal requires more refactoring work, and I do not recommend it at this
> > time.
>
> I didn't see any difficulties there, so I've already made
> explicitOrDefaultValue private.
>
>
> > The "TODO: Some callers..." comment will need to be
> > adjusted as well.
>
> I have removed the "TODO" comment entirely since access to the storage
> variable is now properly protected via accessors.
>
> Kind regards,
> Ankor.
>
> =D0=B2=D1=82, 21 =D0=B0=D0=BF=D1=80. 2026=E2=80=AF=D0=B3. =D0=B2 17:47, A=
lex Rousskov <
> [email protected]>:
>
>> On 2026-04-21 04:01, Amos Jeffries wrote:
>> > I do not understand why we need to store annotations in their
>> > serialized format in the first place.
>>
>> Today, we store "serialized" annotations because different ACLs may
>> specify different delimiters for the same imported annotation value.
>> There are two reasonable ways to look at the current design:
>>
>> A: This design is broken: Deserialization should happen at annotation
>> import time (e.g., in helper response parser). Something like "-m"
>> belongs to helper configuration, not ACLs. 2015 commit 76ee67ac was too
>> focused on its "_match_ substrings of _existing_ annotations" use cases
>> to realize that this is an import feature, not a matching feature.
>>
>> B: This design is correct: Future ACLs may specify match regexes or even
>> custom matching functions that would interpret imported values
>> (effectively deserializign them) differently in different contexts. For
>> example, a helper may send "metadata_" annotation for a denied user
>> using a different syntax than the one used for "metadata_" annotation
>> for an allowed user. 2015 commit 76ee67ac got it right: We should place
>> "-m" inside ACLs, just like we place "-i" inside ACLs. If helpers want
>> Squid to import a list of values, we should add such support, but that
>> is a separate matter.
>>
>>
>> >>> 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 escapin=
g
>> >>> 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=3Dgroup1 group=3Dgroup2 group=3Dgroup3
>> >>
>> >
>> > No. That will be added as three different kv-pair by the helper logic.
>>
>> If the above does add three different name=3Dvalue annotations with the
>> same name, then the answer to my question is "Yes" rather than "No".
>>
>> I did _not_ test this, but, after looking at
>> Helper::Reply::parseResponseKeys() code, I suspect that, yes, "the same
>> effect be achieved today by sending a helper response containing
>> multiple same-name annotations". If that untested theory is correct,
>> then we do not need an additional hack to allow helpers to send Squid a
>> list of same-name values.
>>
>> Alex.
>>
>> _______________________________________________
>> squid-dev mailing list
>> [email protected]
>> https://lists.squid-cache.org/listinfo/squid-dev
>>
>
--000000000000084e330652f181bc
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr">Hello Alex and Amos,<br><br>I would like to follow up on o=
ur previous discussion regarding this issue.<br><br>> > Regarding the=
warning for admins:<br>> =C2=A0 ><br>> =C2=A0 > =C2=A0> >=
; We can also add code to warn admins (via a cache.log WARNING message)<br>=
> =C2=A0 > =C2=A0> > when a "note" ACL configured wit=
hout "-m" looks at an annotation value<br>> =C2=A0 > =C2=A0=
> > containing a comma, but that requires more work.<br>> =C2=A0 &=
gt;<br>> =C2=A0 > I believe this warning might be useful for the curr=
ent buggy version.<br>> =C2=A0 > However, with this fix, the behavior=
will correctly follow the<br>> =C2=A0 > documentation, so a warning =
shouldn't be necessary in the patched version.<br>> <br>> =C2=A0 =
That warning would be useful in several cases. For example, it would be<br>=
> =C2=A0 useful for an admin of a used-to-work-before-the-upgrade-Squid =
who does<br>> =C2=A0 not realize that their helper is returning a list o=
f values and that<br>> =C2=A0 they now must start using "-m" f=
or their upgraded Squid to (continue to)<br>> =C2=A0 handle that case co=
rrectly.<br>> <br>> =C2=A0 AFAICT, the biggest problem with that warn=
ing is that admins of helpers<br>> =C2=A0 that return values with commas=
that should _not_ use "-m" (because those<br>> =C2=A0 commas =
are not used as sub-value separators) would have to manually<br>> =C2=A0=
disable the warning (via cache_log_message or, if we do even more extra<br=
>> =C2=A0 work, via something like "+m" in individual ACL defi=
nitions).<br><br><br>I have prepared a new PR that implements warnings when=
the '-m' flag is missing from the note ACL while the helper's =
response contains commas. It also introduces the '+m' flag to expli=
citly disable splitting comma-separated annotation values and suppress thes=
e warnings.<br><br>May I proceed with publishing it on GitHub?<br><br>I sug=
gest backporting this patch to Squid 7.x and including it in a few upcoming=
Squid 8.x releases. After a few cycles, we can remove these warnings from =
the codebase. Since they are intended to be short-lived across only a few v=
ersions, I also suggest omitting them from doc/debug-messages.dox.<br><br>A=
dditionally, the '+m' option should probably remain undocumented, a=
s it will become redundant once the bugfix in PR #2410 is fully applied (as=
suming it will be included in Squid 8 releases).<div><br></div><div>Kind re=
gards,</div><div>=C2=A0 =C2=A0 Ankor.</div></div><br><div class=3D"gmail_qu=
ote gmail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">=D1=81=D1=
=80, 22 =D0=B0=D0=BF=D1=80. 2026=E2=80=AF=D0=B3. =D0=B2 12:46, Andrey K <=
;<a href=3D"mailto:[email protected]">[email protected]</a>>:<br></d=
iv><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;bord=
er-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir=3D"ltr">Hello=
, Alex and Amos,<div><br></div><div>I have submitted the PR:=C2=A0<a href=
=3D"https://github.com/squid-cache/squid/pull/2410" target=3D"_blank">https=
://github.com/squid-cache/squid/pull/2410</a></div><div><br></div><div>Alex=
, thank you very much for your recommendations regarding the code refactori=
ng.</div><div><br></div><div>> Ideally, explicitOrDefaultValue should be=
private, but achieving that<br>> ideal requires more refactoring work, =
and I do not recommend it at this<br>> time.</div><div><br></div><div>I =
didn't see any difficulties there, so I've already made=C2=A0<font =
face=3D"monospace">explicitOrDefaultValue</font>=C2=A0private.<br><br><br>&=
gt; The "TODO: Some callers..." comment will need to be<br>> a=
djusted as well.</div><div><br></div><div>I have removed the=C2=A0<font fac=
e=3D"monospace">"TODO"</font>=C2=A0comment entirely since access =
to the storage variable is now properly protected via accessors.</div><div>=
<br></div><div>Kind regards,</div><div>=C2=A0 =C2=A0 Ankor.</div></div><br>=
<div class=3D"gmail_quote"><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 17:47, Alex Roussko=
v <<a href=3D"mailto:[email protected]" target=3D"_blank"=
>[email protected]</a>>:<br></div><blockquote class=3D"gm=
ail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,=
204,204);padding-left:1ex">On 2026-04-21 04:01, Amos Jeffries wrote:<br>
> I do not understand why we need to store annotations in their <br>
> serialized format in the first place.<br>
<br>
Today, we store "serialized" annotations because different ACLs m=
ay <br>
specify different delimiters for the same imported annotation value. <br>
There are two reasonable ways to look at the current design:<br>
<br>
A: This design is broken: Deserialization should happen at annotation <br>
import time (e.g., in helper response parser). Something like "-m"=
; <br>
belongs to helper configuration, not ACLs. 2015 commit 76ee67ac was too <br=
>
focused on its "_match_ substrings of _existing_ annotations" use=
cases <br>
to realize that this is an import feature, not a matching feature.<br>
<br>
B: This design is correct: Future ACLs may specify match regexes or even <b=
r>
custom matching functions that would interpret imported values <br>
(effectively deserializign them) differently in different contexts. For <br=
>
example, a helper may send "metadata_" annotation for a denied us=
er <br>
using a different syntax than the one used for "metadata_" annota=
tion <br>
for an allowed user. 2015 commit 76ee67ac got it right: We should place <br=
>
"-m" inside ACLs, just like we place "-i" inside ACLs. =
If helpers want <br>
Squid to import a list of values, we should add such support, but that <br>
is a separate matter.<br>
<br>
<br>
>>> 2. Supporting custom delimiters in helper responses.<br>
>>> I also propose a PR to support a format where tag values can b=
e <br>
>>> passed as a list with a custom delimiter:<br>
>>> =C2=A0=C2=A0 =C2=A0 <key>=3D<delimiter>"<v=
alue1><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 th=
e <br>
>>> specified custom delimiter, while still supporting delimiter e=
scaping <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 i=
n <br>
>> this context. Overloading quotation meaning would be confusing/wro=
ng.<br>
>><br>
>> Overall, I am not excited about this hack, but let's start wit=
h these <br>
>> questions about its scope:<br>
>><br>
>> * Can the same effect be achieved today by sending a helper respon=
se <br>
>> containing multiple same-name annotations? For example:<br>
>><br>
>> =C2=A0=C2=A0=C2=A0=C2=A0 group=3Dgroup1 group=3Dgroup2 group=3Dgro=
up3<br>
>><br>
> <br>
> No. That will be added as three different kv-pair by the helper logic.=
<br>
<br>
If the above does add three different name=3Dvalue annotations with the <br=
>
same name, then the answer to my question is "Yes" rather than &q=
uot;No".<br>
<br>
I did _not_ test this, but, after looking at <br>
Helper::Reply::parseResponseKeys() code, I suspect that, yes, "the sam=
e <br>
effect be achieved today by sending a helper response containing <br>
multiple same-name annotations". If that untested theory is correct, <=
br>
then we do not need an additional hack to allow helpers to send Squid a <br=
>
list of same-name values.<br>
<br>
Alex.<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>
</blockquote></div>
--000000000000084e330652f181bc--
--===============2306553263455119260==
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
--===============2306553263455119260==--