|
| 1 | +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> |
| 2 | +<HTML> |
| 3 | + <HEAD> |
| 4 | + <TITLE> [squid-dev] Issue with acl note (without -m) splitting helper tokens containing commas |
| 5 | + </TITLE> |
| 6 | + <LINK REL="Index" HREF="index.html" > |
| 7 | + <LINK REL="made" HREF="mailto:squid-dev%40lists.squid-cache.org?Subject=Re%3A%20%5Bsquid-dev%5D%20Issue%20with%20acl%20note%20%28without%20-m%29%20splitting%20helper%0A%20tokens%20containing%20commas&In-Reply-To=%3C3d5b3018-9757-4e7d-bb24-172c674c2359%40treenet.co.nz%3E"> |
| 8 | + <META NAME="robots" CONTENT="index,nofollow"> |
| 9 | + <style type="text/css"> |
| 10 | + pre { |
| 11 | + white-space: pre-wrap; /* css-2.1, curent FF, Opera, Safari */ |
| 12 | + } |
| 13 | + </style> |
| 14 | + <META http-equiv="Content-Type" content="text/html; charset=us-ascii"> |
| 15 | + <LINK REL="Previous" HREF="010022.html"> |
| 16 | + <LINK REL="Next" HREF="010025.html"> |
| 17 | + </HEAD> |
| 18 | + <BODY BGCOLOR="#ffffff"> |
| 19 | + <H1>[squid-dev] Issue with acl note (without -m) splitting helper tokens containing commas</H1> |
| 20 | + <B>Amos Jeffries</B> |
| 21 | + <A HREF="mailto:squid-dev%40lists.squid-cache.org?Subject=Re%3A%20%5Bsquid-dev%5D%20Issue%20with%20acl%20note%20%28without%20-m%29%20splitting%20helper%0A%20tokens%20containing%20commas&In-Reply-To=%3C3d5b3018-9757-4e7d-bb24-172c674c2359%40treenet.co.nz%3E" |
| 22 | + TITLE="[squid-dev] Issue with acl note (without -m) splitting helper tokens containing commas">squid3 at treenet.co.nz |
| 23 | + </A><BR> |
| 24 | + <I>Tue Apr 21 08:01:54 UTC 2026</I> |
| 25 | + <P><UL> |
| 26 | + <LI>Previous message (by thread): <A HREF="010022.html">[squid-dev] Issue with acl note (without -m) splitting helper tokens containing commas |
| 27 | +</A></li> |
| 28 | + <LI>Next message (by thread): <A HREF="010025.html">[squid-dev] Issue with acl note (without -m) splitting helper tokens containing commas |
| 29 | +</A></li> |
| 30 | + <LI> <B>Messages sorted by:</B> |
| 31 | + <a href="date.html#10024">[ date ]</a> |
| 32 | + <a href="thread.html#10024">[ thread ]</a> |
| 33 | + <a href="subject.html#10024">[ subject ]</a> |
| 34 | + <a href="author.html#10024">[ author ]</a> |
| 35 | + </LI> |
| 36 | + </UL> |
| 37 | + <HR> |
| 38 | +<!--beginarticle--> |
| 39 | +<PRE>On 21/04/2026 06:59, Alex Rousskov wrote: |
| 40 | +><i> On 2026-04-17 09:08, Andrey K wrote: |
| 41 | +</I>><i> |
| 42 | +</I>>><i> While working with annotations, I’ve noticed an inconsistency in how |
| 43 | +</I>>><i> acl note (without the -m option) handles tokens received from helpers |
| 44 | +</I>>><i> when they contain a comma. |
| 45 | +</I>><i> |
| 46 | +</I>><i> I think it is important to note that there are several distinct players |
| 47 | +</I>><i> here, including: |
| 48 | +</I>><i> |
| 49 | +</I>><i> 1. What annotations the helper sends to Squid. |
| 50 | +</I>><i> |
| 51 | +</I> |
| 52 | +Which is: |
| 53 | + |
| 54 | + group="Staff:accountants,lawyers,security" |
| 55 | + |
| 56 | + |
| 57 | +><i> 2. How helper response parser converts received helper response into |
| 58 | +</I>><i>    transaction annotations. |
| 59 | +</I>><i> |
| 60 | +</I> |
| 61 | +Which is: |
| 62 | + |
| 63 | + 1) remove double-quotes |
| 64 | + 2) translate \-escapes within quoted-string values |
| 65 | + 3) translate %-coded within token values |
| 66 | + |
| 67 | +Nothing more. ',' is not special for that parser. |
| 68 | + |
| 69 | + |
| 70 | +><i> 3. How the "note" ACL code interprets transaction annotations. |
| 71 | +</I>><i>    These annotations may come from sources other than a helper. |
| 72 | +</I>><i> |
| 73 | +</I> |
| 74 | +This is where ',' becomes special as a list delimiter. |
| 75 | + |
| 76 | +The annotation system interprets "a,b,c" as a set of three values, |
| 77 | +stored as a list. |
| 78 | + |
| 79 | + (I do not understand why we need to store annotations in their |
| 80 | +serialized format in the first place. It is generally a bad design.) |
| 81 | + |
| 82 | + |
| 83 | +><i> |
| 84 | +</I>>><i> According to the documentation, an ACL like this: |
| 85 | +</I>>><i>      acl staff note group Staff:accountants,lawyers,security |
| 86 | +</I>>><i> should match a helper response such as: |
| 87 | +</I>>><i>      group="Staff:accountants,lawyers,security" |
| 88 | +</I>><i> |
| 89 | +</I>><i> The above implies that helper response parser should not split group=X |
| 90 | +</I>><i> response fields (using comma as a delimiter). Do we document that |
| 91 | +</I>><i> anywhere? |
| 92 | +</I> |
| 93 | +The only thing the helper response parser does is remove the DQUOTE |
| 94 | +characters around the value string. |
| 95 | + |
| 96 | +As you noted below, the problem is ACL logic interaction with the |
| 97 | +annotation storage. |
| 98 | + |
| 99 | + |
| 100 | +><i> Did our helper response parser ever split such fields in the |
| 101 | +</I>><i> past? |
| 102 | +</I>><i> |
| 103 | +</I> |
| 104 | +If it did that was a bug. Prior to the annotations feature we did not |
| 105 | +support key=value-list, only key=value (singular value). |
| 106 | + |
| 107 | + |
| 108 | + |
| 109 | +><i> BTW, 2015 commit 76ee67ac used a very similar example. AFAICT by looking |
| 110 | +</I>><i> at that code, we did not apply value delimiters by default back then |
| 111 | +</I>><i> (i.e. when ACL_F_SUBSTRING a.k.a. "-m" flag was not set). The bug was |
| 112 | +</I>><i> introduced in 2017 commit 4eac3407 that replaced a possibly-nil |
| 113 | +</I>><i> `flags.delimiters()` with a never-nil `&delimiters.value`. |
| 114 | +</I>><i> |
| 115 | +</I>><i> The following comment suggests that we missed the fact that using the |
| 116 | +</I>><i> [default-initialized] value "without checking whether the option is |
| 117 | +</I>><i> enabled()" is a bug -- the corresponding "trick" never fully worked: |
| 118 | +</I>><i> |
| 119 | +</I>><i> ```C++ |
| 120 | +</I>><i> // TODO: Some callers use .value without checking whether the option is |
| 121 | +</I>><i> // enabled(), accessing the (default-initialized or customized) default |
| 122 | +</I>><i> // value that way. This trick will stop working if we add valued options |
| 123 | +</I>><i> // that can be disabled (e.g., --with-foo=x --without-foo). |
| 124 | +</I>><i> ``` |
| 125 | +</I>><i> |
| 126 | +</I>><i> |
| 127 | +</I>>><i> However, this is not the case. The helper's response is split into |
| 128 | +</I>>><i> tokens using a comma as the default delimiter. As a result, only ACLs |
| 129 | +</I>>><i> like the following will match: |
| 130 | +</I>>><i>      acl staff note group lawyers |
| 131 | +</I>>><i> |
| 132 | +</I>>><i> This behavior occurs because in Acl::NoteCheck::matchNotes(), a comma |
| 133 | +</I>>><i> is passed as the default delimiter to the expandListEntries() function |
| 134 | +</I>><i> |
| 135 | +</I>><i> Agreed. |
| 136 | +</I>><i> |
| 137 | +</I> |
| 138 | +Nod. |
| 139 | + |
| 140 | +><i> |
| 141 | +</I>>><i> I would like to propose two changes: |
| 142 | +</I>>><i> 1. Removing the default comma delimiter. |
| 143 | +</I>><i> |
| 144 | +</I>><i> ... and check enabled() before using the stored value, fixing the bug |
| 145 | +</I>><i> introduced in 2017 commit 4eac3407. |
| 146 | +</I>><i> |
| 147 | +</I> |
| 148 | +IMO that is the initial bug causing issues. |
| 149 | + |
| 150 | + |
| 151 | + |
| 152 | +><i> |
| 153 | +</I>>><i> I am prepared to submit a simple PR to exclude this comma to fix the |
| 154 | +</I>>><i> incorrect matching of strings containing commas. |
| 155 | +</I>>><i> However, I realize this might be a breaking change for users who |
| 156 | +</I>>><i> currently rely on this implicit splitting behavior. |
| 157 | +</I>><i> |
| 158 | +</I>><i> Yes. We should disclose the bug fix in Squid release notes. |
| 159 | +</I>><i> |
| 160 | +</I>><i> We can also add code to warn admins (via a cache.log WARNING message) |
| 161 | +</I>><i> when a "note" ACL configured without "-m" looks at an annotation value |
| 162 | +</I>><i> containing a comma, but that requires more work. |
| 163 | +</I>><i> |
| 164 | +</I>><i> |
| 165 | +</I>>><i> 2. Supporting custom delimiters in helper responses. |
| 166 | +</I>>><i> I also propose a PR to support a format where tag values can be passed |
| 167 | +</I>>><i> as a list with a custom delimiter: |
| 168 | +</I>>><i>      <key>=<delimiter>"<value1><delimiter><value2>..." |
| 169 | +</I>>><i> For example: |
| 170 | +</I>>><i>      group=,"group1,group2,group3" |
| 171 | +</I>>><i>      clt_con_tag=;"tag1;tag2;tag3" |
| 172 | +</I>>><i> In this PR, the helper response would be tokenized based on the |
| 173 | +</I>>><i> specified custom delimiter, while still supporting delimiter escaping |
| 174 | +</I>>><i> with a backslash (\). |
| 175 | +</I>><i> |
| 176 | +</I>><i> I do not think this hack will work well as is, without syntax |
| 177 | +</I>><i> modifications because Squid already uses double quotes specially in this |
| 178 | +</I>><i> context. Overloading quotation meaning would be confusing/wrong. |
| 179 | +</I>><i> |
| 180 | +</I>><i> Overall, I am not excited about this hack, but let's start with these |
| 181 | +</I>><i> questions about its scope: |
| 182 | +</I>><i> |
| 183 | +</I>><i> * Can the same effect be achieved today by sending a helper response |
| 184 | +</I>><i> containing multiple same-name annotations? For example: |
| 185 | +</I>><i> |
| 186 | +</I>><i>     group=group1 group=group2 group=group3 |
| 187 | +</I>><i> |
| 188 | +</I> |
| 189 | +No. That will be added as three different kv-pair by the helper logic. |
| 190 | + |
| 191 | +The annotations storing kv-pair in MiME syntax with mixed arbitrary |
| 192 | +key=value and key=list,of,values is a problem. |
| 193 | + |
| 194 | + |
| 195 | +><i> * If the "note" ACL bug is fixed, do we still need to allow helper to |
| 196 | +</I>><i> use custom value delimiters? |
| 197 | +</I>><i> |
| 198 | +</I>><i> * What would Squid do today if it receives a `group=,"a,b"` annotation |
| 199 | +</I>><i> from a helper? AFAICT from looking at |
| 200 | +</I>><i> Helper::Reply::parseResponseKeys(), Squid would silently treat the |
| 201 | +</I>><i> leading comma delimiter as the first character of the received |
| 202 | +</I>><i> annotation value and keep double quotes, right? |
| 203 | +</I> |
| 204 | +No. All double-quotes are removed. |
| 205 | + |
| 206 | +That input would reach the annotations storage as: |
| 207 | + { key: 'group', value: ',a,b' } |
| 208 | + |
| 209 | +Then the annotation storage saves that as key=value,list syntax and |
| 210 | +expands it later. Same problems all over again. |
| 211 | + |
| 212 | + |
| 213 | +><i> |
| 214 | +</I>><i> * Squid does not treat backslashes in annotation values specially today, |
| 215 | +</I>><i> does it? If present, they become part of the annotation key or value, |
| 216 | +</I>><i> right? |
| 217 | +</I> |
| 218 | +No. They are translated by the helper response parser into octets. |
| 219 | + |
| 220 | + |
| 221 | +Input ' group="a\,b" ' would reach the annotations storage as: |
| 222 | + { key: 'group', value: 'a,b' } |
| 223 | + |
| 224 | + |
| 225 | +The bug is not in the helper protocol. It does **nothing** with comma. |
| 226 | +Correctly so IMO. |
| 227 | + |
| 228 | + |
| 229 | +HTH |
| 230 | +Amos |
| 231 | + |
| 232 | +</PRE> |
| 233 | + |
| 234 | + |
| 235 | +<!--endarticle--> |
| 236 | + <HR> |
| 237 | + <P><UL> |
| 238 | + <!--threads--> |
| 239 | + <LI>Previous message (by thread): <A HREF="010022.html">[squid-dev] Issue with acl note (without -m) splitting helper tokens containing commas |
| 240 | +</A></li> |
| 241 | + <LI>Next message (by thread): <A HREF="010025.html">[squid-dev] Issue with acl note (without -m) splitting helper tokens containing commas |
| 242 | +</A></li> |
| 243 | + <LI> <B>Messages sorted by:</B> |
| 244 | + <a href="date.html#10024">[ date ]</a> |
| 245 | + <a href="thread.html#10024">[ thread ]</a> |
| 246 | + <a href="subject.html#10024">[ subject ]</a> |
| 247 | + <a href="author.html#10024">[ author ]</a> |
| 248 | + </LI> |
| 249 | + </UL> |
| 250 | + |
| 251 | +<hr> |
| 252 | +<a href="https://lists.squid-cache.org/listinfo/squid-dev">More information about the squid-dev |
| 253 | +mailing list</a><br> |
| 254 | +</body></html> |
0 commit comments