[squid-dev] [PATCH] Bug 1961 pt2: store URI userinfo (aka login) in class URL

[Amos Jeffries]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 13/05/2014 12:24 p.m., Alex Rousskov wrote:
> On 05/09/2014 01:31 PM, Amos Jeffries wrote:
> 
>> +    const SBuf &userInfo(void) const {return userInfo_;}
> 
> Please avoid "(void)" in new code and use "()" for consistency with
> most C++ code.
> 

Done.

> 
>> +    if (checklist->request->url.userInfo().isEmpty()) { +
>> debugs(28, 5, "URL has no user-info details. cannot match"); +
>> return 0; // nothing can match +    }
> 
> RFC 3986 BNF implies that empty "userinfo" is valid:
> 
>> authority   = [ userinfo "@" ] host [ ":" port ] userinfo    = *(
>> unreserved / pct-encoded / sub-delims / ":" )
> 
> AFAICT, Squid does not include the "@" character in userinfo,
> right? If the old code could not fully handle a URL with empty but
> present userinfo (e.g., could not match it using a urllogin ACL
> that searches for an empty userinfo), then this is not a problem,
> although adding an XXX comment would be nice.

It is valid URL syntax. But as the debug message indicates no-name
will never match on a ACL list of existing names.

There is no XXX or TODO that can be done there IMO. The ACL
appropriate for matching no-userinfo URLs is "url_regex
^[a-zA-Z\+]+://@" not the urllogin ACL which can trivially be confused
with empty vs absent userinfo.


> 
>> +    SBuf::size_type len =
>> checklist->request->url.userInfo().copy(str, sizeof(str)-1); +
>> str[len] = '\0'; +    rfc1738_unescape(str); +    int result =
>> data->match(str);
> 
> Please make "len" and "result" constant if possible.

Done for len. Got rid of result.

> 
> 
>> +    /* If there was a username part. +     * Ignore 0-length
>> usernames, retain what we have already +     */ +    if (colonPos
>> != 0) {
> 
> 
> The code is too smart even though it is correct. I suggest adding
> an explicit npos condition, even though it is not technically
> needed:
> 
> // If there was a non-empty username part, retain it. if (colonPos
> == SBuf::npos || colonPos > 0) { ... }
> 

Done.

> 
>> +   // URI userinfo/login is deprecated by IETF so we dont care
>> if it crops accidentally +   len =
>> request->url.userInfo().copy(loginbuf, min(len,
>> sizeof(loginbuf)-2));
> 
> I do not think that comment is a valid statement. With all due
> respect for IETF, official deprecation is not enough to make us not
> care about mishandling real-world traffic. If Squid can crop the
> deprecated userinfo when forwarding or logging it, then it is a
> problem we should care about. However, you do not have to fix that
> problem if it was already present in the code. Documenting it with
> XXX is fine, especially if it is difficult to fix!
> 

Removed this whole double-copy operation and used SQUIDSBUFPRINT() in
the snprintf() statement. So we do not crop at all now unless the
userinfo pushes the total URL over maximum URI length.


> 
> The above changes can be done during commit IMO. I am unable to
> fully audit the parsing changes, but if you are happy with them,
> that is enough for me.
> 

Applied as rev.13684 with some polishing to follow trunk changes.


Amos

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (MingW32)

iQEcBAEBAgAGBQJUWJPHAAoJELJo5wb/XPRj2tMH/i/m+9MbYKym/6741bq0IU4M
C+3bTxCD83Mh3nL9/rk+UgzKF333NQI6KgLXce/5FrbeaRHdK7WNriDVBnIAyPvG
UvGnM1NsBlXcuU+6y2aXH+5p6atG2NI6jVJUe22vKUGotRJ7PO+SkhNGVosIzCBv
RJencHWOfINqYT2ptn5rag9ZKYBDsVG28nKWpfhmfVjHHQ58EsD+xawDfqQ5t/JV
qEn7jmV9ewfhH7bjWDIuHnQSRKCtMrbR4z1QU8lzh2F7XOvegefl8lK5CHv7XvgI
lyA2he97sB1RnOpv2z7k7uacL738LDFI2L0I8DF4pJPw3FtBiRYiNwF2azam+Zs=
=tFqp
-----END PGP SIGNATURE-----