[squid-dev] [PATCH] Fix ext_session_acl to handle - when no argument is passed

[Alex Rousskov]

On 03/22/2017 07:53 PM, Amos Jeffries wrote:

> This helpers' passive mode does not need to care what the input is, so
> long as it is consistent.

I do not recommend ignoring input format in hope that ignored input is
consistent/acceptable/compatible/etc. However, I am not going to object
to any changes to this helper, even if they violate that general
principle, so feel free to commit whatever you think is the best. I do
not plan on following this thread further.


> The admin may still use %DATA in either mode, so not touching the input
> seems to be the better way to go except where there is no choice.

Using %DATA explicitly at the expected location (i.e., last) is not a
problem. Using it in unexpected and possibly unsupported places is.


>> I would expect something along these lines instead (pseudo code):
>>
>>   // What action was requested (including no/default action)?
>>   if (... LOGIN ...) action = 1;
>>   else if (... LOGOUT ...) action = -1;
>>   else if (... - ...) action = default_action;
>>   else { ... BH unexpected query format ...; continue; }
>>
> 
> That will result in BH for passive mode when the admin wants to use
> %DATA in the format string (unless it is last).

Yes, and I think that would be the right behavior for the helper.

The only counter argument that I can think of is that undocumented(?)
passive helper functionality correctly uses (not just tolerates/ignores
but _uses_) arbitrary query input past the source address. It is
difficult for me to imagine a good helper design that would allow for
such a thing but that does not mean it cannot exist. Please note that
this counter argument does not apply to the active helper where the
action ought to be required even when arbitrary query input is used.


> The "// else do nothing" is explicitly added by my patch to document
> that the case of unknown other values is valid/expected and to not touch
> the contents of 'detail' when it happens.

The comment does not explain _why_ we should do nothing so it is 50%
useless because it just retells what the code is already saying. The 50%
usefulness comes from relaying the fact that the developer actually
thought about the non-obvious "else" case (but then decided to hide his
reasons for doing nothing in that case).


Cheers,

Alex.