Fixes for addressbook-query filters

[mstilkerich]

A null dereference occurs when a param-filter is checked on a property that does not have this parameter set. I adapted the tests by simply rearranging the test data set so that a TEL property without TYPE parameter comes before those with TYPE parameters to trigger the situation in the existing tests.

A log trace for your reference:

>>>>>>>>
REPORT /dav.php/addressbooks/mikey/default/ HTTP/1.1
Content-Length: 564
User-Agent: GuzzleHttp/7
Host: baikal.mike2k.de
Depth: 1
Content-Type: application/xml; charset=UTF-8

<?xml version="1.0"?>
<CARDDAV:addressbook-query xmlns:DAV="DAV:" xmlns:CARDDAV="urn:ietf:params:xml:ns:carddav" xmlns:CS="http://calendarserver.org/ns/">
 <DAV:prop>
  <DAV:getetag/>
  <CARDDAV:address-data/>
 </DAV:prop>
 <CARDDAV:filter test="anyof">
  <CARDDAV:prop-filter name="EMAIL" test="anyof">
   <CARDDAV:param-filter name="TYPE">
    <CARDDAV:text-match negate-condition="no" collation="i;unicode-casemap" match-type="equals">HOME</CARDDAV:text-match>
   </CARDDAV:param-filter>
  </CARDDAV:prop-filter>
 </CARDDAV:filter>
</CARDDAV:addressbook-query>

<<<<<<<<
HTTP/1.1 500 Internal Server Error
Date: Fri, 22 Jan 2021 16:12:53 GMT
Server: Apache/2.4.41 (Ubuntu)
WWW-Authenticate: Negotiate oYG3MIG0oAMKAQChCwYJKoZIhvcSAQICooGfBIGcYIGZBgkqhkiG9xIBAgICAG+BiTCBhqADAgEFoQMCAQ+iejB4oAMCARKicQRvPTYAJnGyjtnnnZ0mKrUTGmfrv16FDm6AOfELSH+Tr3gTActMMYpP0JsATIG+z3hNZ0cohyF9ovHkI4SnHfCu6XraiwezWRXUGN9pof70KKtCYZIjS2Ox+kVmh2ymZVuhzPteimH62rl5yViBSORq
Set-Cookie: PHPSESSID=5s804cp5rhd6r2i9iiv7aarfeq; path=/; HttpOnly
Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate
Pragma: no-cache
X-Sabre-Version: 4.1.2
Content-Length: 262
Connection: close
Content-Type: application/xml; charset=utf-8

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:sabredav-version>4.1.2</s:sabredav-version>
  <s:exception>Error</s:exception>
  <s:message>Call to a member function getValue() on null</s:message>
</d:error>

[codecov]

Codecov Report

Merging #1322 (b5792d3) into master (82ea0e9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #1322   +/-   ##
=========================================
  Coverage     97.12%   97.13%           
- Complexity     2788     2789    +1     
=========================================
  Files           174      174           
  Lines          8045     8050    +5     
=========================================
+ Hits           7814     7819    +5     
  Misses          231      231           

Impacted Files	Coverage Δ	Complexity Δ
lib/CardDAV/Plugin.php	96.92% <100.00%> (+0.04%)	122.00 <0.00> (+1.00)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82ea0e9...b5792d3. Read the comment docs.

[mstilkerich]

There is another issue in the same function. A negated text-match should match when a parameter value is found that does not match the search string. However, the current code requires there is no such parameter value. This is a problem if there are multiple instances of the property, where at least one matches the non-negated text-match filter, but there is also another one that matches the negated text-match filter. In this case, the function would filter out the card, because it searches for a property that matches the non-negated filter, and then stops and inverts the end result.

Example:

BEGIN:VCARD
VERSION:3.0
UID:3151DE6A-BC35-4612-B340-B53A034A2B27
FN:First Last
N:Last;First;;;
EMAIL;TYPE=HOME:home@example.com
EMAIL;TYPE=WORK:work@example.com
END:VCARD

A <prop-filter name="EMAIL"><param-filter name="TYPE"><text-match negate-condition="yes">HOME</text-match></param-filter></prop-filter> should match the card, because there is an EMAIL property that has no TYPE attribute that matches (the WORK address). But the current code returns false, because it finds a property that does match the non-negated filter (the HOME address), and then stops and inverts the result.

Relevant parts of RFC 6352:

Description:  The CARDDAV:prop-filter XML element specifies search criteria on a specific vCard property
       (e.g., "NICKNAME").  An address object is said to match a CARDDAV:prop-filter if:
      *  A vCard property of the type specified by the "name" attribute exists, and the CARDDAV:prop-filter is
         empty, or it matches any specified CARDDAV:text-match or CARDDAV:param-filter conditions.  The
        "test" attribute specifies whether any (logical OR) or all (logical AND) of the text-filter and
         param-filter tests need to match in order for the overall filter to match.
or:
      *  [...]

Description:  The CARDDAV:param-filter XML element specifies search criteria on a specific vCard
       property parameter (e.g., TYPE) in the scope of a given CARDDAV:prop-filter.  A vCard property
       is said to match a CARDDAV:param-filter if:
      *  A parameter of the type specified by the "name" attribute exists, and the
         CARDDAV:param-filter is empty, or it matches the CARDDAV:text-match conditions if specified.
or:
      *  [...]

[mstilkerich]

There appears to be the same problem with negated text-matches at the prop-filter level. Fixed that as well.

[phil-davis]

LGTM - tests fail before the changes, and pass with the code changes, good stuff.

[phil-davis]

I rebased. Let's see how many hours delay Travis has today!