Require <getetag> eTag values

rfc2822 · rfc2822 · commit c11ebb6473a7 · 2023-01-09T16:19:13.000+01:00
- introduce XmlUtils.requireReadText()
- Property.parse(): ignore invalid properties
- GetETag: require input text so that the value fields don't have to be nullable
diff --git a/src/main/kotlin/at/bitfire/dav4jvm/Property.kt b/src/main/kotlin/at/bitfire/dav4jvm/Property.kt
@@ -7,9 +7,11 @@
 package at.bitfire.dav4jvm
 
 import at.bitfire.dav4jvm.Dav4jvm.log
+import at.bitfire.dav4jvm.exception.InvalidPropertyException
 import org.xmlpull.v1.XmlPullParser
 import java.io.Serializable
 import java.util.*
+import java.util.logging.Level
 
 /**
  * Represents a WebDAV property.
@@ -20,8 +22,8 @@ import java.util.*
 interface Property {
 
     data class Name(
-            val namespace: String,
-            val name: String
+        val namespace: String,
+        val name: String
     ): Serializable {
 
         override fun toString() = "$namespace:$name"
@@ -39,14 +41,19 @@ interface Property {
             while (!(eventType == XmlPullParser.END_TAG && parser.depth == depth)) {
                 if (eventType == XmlPullParser.START_TAG && parser.depth == depth + 1) {
                     val depthBeforeParsing = parser.depth
-                    val name = Property.Name(parser.namespace, parser.name)
-                    val property = PropertyRegistry.create(name, parser)
-                    assert(parser.depth == depthBeforeParsing)
-
-                    if (property != null) {
-                        properties.add(property)
-                    } else
-                        log.fine("Ignoring unknown property $name")
+                    val name = Name(parser.namespace, parser.name)
+
+                    try {
+                        val property = PropertyRegistry.create(name, parser)
+                        assert(parser.depth == depthBeforeParsing)
+
+                        if (property != null) {
+                            properties.add(property)
+                        } else
+                            log.fine("Ignoring unknown property $name")
+                    } catch (e: InvalidPropertyException) {
+                        log.log(Level.WARNING, "Ignoring invalid property", e)
+                    }
                 }
                 eventType = parser.next()
             }
diff --git a/src/main/kotlin/at/bitfire/dav4jvm/XmlUtils.kt b/src/main/kotlin/at/bitfire/dav4jvm/XmlUtils.kt
@@ -6,6 +6,7 @@
 
 package at.bitfire.dav4jvm
 
+import at.bitfire.dav4jvm.exception.InvalidPropertyException
 import org.xmlpull.v1.XmlPullParser
 import org.xmlpull.v1.XmlPullParserException
 import org.xmlpull.v1.XmlPullParserFactory
@@ -60,6 +61,16 @@ object XmlUtils {
         return text
     }
 
+    /**
+     * Same as [readText], but requires a [XmlPullParser.TEXT] value.
+     *
+     * @throws InvalidPropertyException when no text could be read
+     */
+    @Throws(InvalidPropertyException::class, IOException::class, XmlPullParserException::class)
+    fun requireReadText(parser: XmlPullParser): String =
+        readText(parser) ?:
+        throw InvalidPropertyException("XML text for ${parser.namespace}:${parser.name} must not be empty")
+
     @Throws(IOException::class, XmlPullParserException::class)
     fun readTextProperty(parser: XmlPullParser, name: Property.Name): String? {
         val depth = parser.depth
diff --git a/src/main/kotlin/at/bitfire/dav4jvm/exception/InvalidPropertyException.kt b/src/main/kotlin/at/bitfire/dav4jvm/exception/InvalidPropertyException.kt
@@ -0,0 +1,8 @@
+package at.bitfire.dav4jvm.exception
+
+/**
+ * Represents an invalid XML (WebDAV) property. This is for instance thrown
+ * when parsing something like `<multistatus>...<getetag><novalue/></getetag>`
+ * because a text value would be expected.
+ */
+class InvalidPropertyException(message: String): Exception(message)
diff --git a/src/main/kotlin/at/bitfire/dav4jvm/property/GetETag.kt b/src/main/kotlin/at/bitfire/dav4jvm/property/GetETag.kt
@@ -20,7 +20,7 @@ import org.xmlpull.v1.XmlPullParser
  * header value to the constructor and then use [eTag] and [weak].
  */
 class GetETag(
-    rawETag: String?
+    rawETag: String
 ): Property {
 
     companion object {
@@ -32,46 +32,52 @@ class GetETag(
     }
 
     /**
-     * The parsed eTag value. May be null if the tag is weak.
+     * The parsed ETag value, excluding the weakness indicator and the quotes.
      */
-    val eTag: String?
+    val eTag: String
 
     /**
-     * If the tag is weak. May be null if the tag passed is null.
+     * Whether the ETag is weak.
      */
-    val weak: Boolean?
+    var weak: Boolean
 
     init {
         /* entity-tag = [ weak ] opaque-tag
            weak       = "W/"
            opaque-tag = quoted-string
         */
-        var tag: String? = rawETag
-        if (tag != null) {
-            // remove trailing "W/"
-            if (tag.startsWith("W/") && tag.length >= 3) {
-                // entity tag is weak
-                tag = tag.substring(2)
-                weak = true
-            } else
-                weak = false
-
-            tag = QuotedStringUtils.decodeQuotedString(tag)
+        var tag = rawETag
+
+        // remove trailing "W/"
+        if (tag.startsWith("W/") && tag.length >= 3) {
+            // entity tag is weak
+            tag = tag.substring(2)
+            weak = true
         } else
-            weak = null
+            weak = false
 
-        eTag = tag
+        eTag = QuotedStringUtils.decodeQuotedString(tag)
     }
 
-    override fun toString() = eTag ?: "(null)"
+    override fun toString() = "ETag(weak=${weak}, tag=$eTag)"
+
+    override fun equals(other: Any?): Boolean {
+        if (other !is GetETag)
+            return false
+        return eTag == other.eTag && weak == other.weak
+    }
+
+    override fun hashCode(): Int {
+        return eTag.hashCode() xor weak.hashCode()
+    }
 
 
     object Factory: PropertyFactory {
 
         override fun getName() = NAME
 
         override fun create(parser: XmlPullParser) =
-            GetETag(XmlUtils.readText(parser))
+            GetETag(XmlUtils.requireReadText(parser))
 
     }
 
diff --git a/src/test/kotlin/at/bitfire/dav4jvm/DavCollectionTest.kt b/src/test/kotlin/at/bitfire/dav4jvm/DavCollectionTest.kt
@@ -105,24 +105,24 @@ class DavCollectionTest {
                     assertTrue(response.isSuccess())
                     assertEquals(Response.HrefRelation.MEMBER, relation)
                     val eTag = response[GetETag::class.java]
-                    assertEquals("00001-abcd1", eTag?.eTag)
-                    assertTrue(eTag?.weak == false)
+                    assertEquals("00001-abcd1", eTag!!.eTag)
+                    assertFalse(eTag.weak)
                     nrCalled++
                 }
                 url.resolve("/dav/vcard.vcf") -> {
                     assertTrue(response.isSuccess())
                     assertEquals(Response.HrefRelation.MEMBER, relation)
                     val eTag = response[GetETag::class.java]
-                    assertEquals("00002-abcd1", eTag?.eTag)
-                    assertTrue(eTag?.weak == false)
+                    assertEquals("00002-abcd1", eTag!!.eTag)
+                    assertFalse(eTag.weak)
                     nrCalled++
                 }
                 url.resolve("/dav/calendar.ics") -> {
                     assertTrue(response.isSuccess())
                     assertEquals(Response.HrefRelation.MEMBER, relation)
                     val eTag = response[GetETag::class.java]
-                    assertEquals("00003-abcd1", eTag?.eTag)
-                    assertTrue(eTag?.weak == false)
+                    assertEquals("00003-abcd1", eTag!!.eTag)
+                    assertFalse(eTag.weak)
                     nrCalled++
                 }
             }
diff --git a/src/test/kotlin/at/bitfire/dav4jvm/DavResourceTest.kt b/src/test/kotlin/at/bitfire/dav4jvm/DavResourceTest.kt
@@ -245,7 +245,7 @@ class DavResourceTest {
 
             val eTag = GetETag.fromResponse(response)
             assertEquals("My Weak ETag", eTag!!.eTag)
-            assertTrue(eTag.weak!!)
+            assertTrue(eTag.weak)
             assertEquals("application/x-test-result".toMediaType(), GetContentType(response.body!!.contentType()!!).type)
         }
         assertTrue(called)
@@ -268,9 +268,9 @@ class DavResourceTest {
         dav.get("*/*", null) { response ->
             called = true
             assertEquals(sampleText, response.body!!.string())
-            val eTag = GetETag(response.header("ETag"))
+            val eTag = GetETag(response.header("ETag")!!)
             assertEquals("StrongETag", eTag.eTag)
-            assertFalse(eTag.weak!!)
+            assertFalse(eTag.weak)
         }
         assertTrue(called)
 
@@ -768,7 +768,7 @@ class DavResourceTest {
         dav.proppatch(
             setProperties = mapOf(Pair(Property.Name("sample", "setThis"), "Some Value")),
             removeProperties = listOf(Property.Name("sample", "removeThis"))
-        ) { response, hrefRelation ->
+        ) { _, hrefRelation ->
             called = true
             assertEquals(Response.HrefRelation.SELF, hrefRelation)
         }
@@ -804,7 +804,7 @@ class DavResourceTest {
             called = true
             val eTag = GetETag.fromResponse(response)!!
             assertEquals("Weak PUT ETag", eTag.eTag)
-            assertTrue(eTag.weak!!)
+            assertTrue(eTag.weak)
             assertEquals(response.request.url, dav.location)
         }
         assertTrue(called)
@@ -871,7 +871,7 @@ class DavResourceTest {
                     "  </response>" +
                     "</multistatus>"))
         var called = false
-        dav.search("<TEST/>") { response, hrefRelation, ->
+        dav.search("<TEST/>") { response, hrefRelation ->
             assertEquals(Response.HrefRelation.SELF, hrefRelation)
             assertEquals("Found something", response[DisplayName::class.java]?.displayName)
             called = true
diff --git a/src/test/kotlin/at/bitfire/dav4jvm/PropertyTest.kt b/src/test/kotlin/at/bitfire/dav4jvm/PropertyTest.kt
@@ -0,0 +1,63 @@
+/*
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+package at.bitfire.dav4jvm
+
+import at.bitfire.dav4jvm.property.GetETag
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Test
+import org.xmlpull.v1.XmlPullParser
+import org.xmlpull.v1.XmlPullParserFactory
+import java.io.StringReader
+
+class PropertyTest {
+
+    @Test
+    fun testParse_InvalidProperty() {
+        val parser = XmlPullParserFactory.newInstance().apply {
+            isNamespaceAware = true
+        }.newPullParser()
+        parser.setInput(StringReader("<multistatus xmlns='DAV:'><getetag/></multistatus>"))
+        do {
+            parser.next()
+        } while (parser.eventType != XmlPullParser.START_TAG && parser.name != "multistatus")
+
+        // we're now at the start of <multistatus>
+        assertEquals(XmlPullParser.START_TAG, parser.eventType)
+        assertEquals("multistatus", parser.name)
+
+        // parse invalid DAV:getetag
+        assertTrue(Property.parse(parser).isEmpty())
+
+        // we're now at the end of <multistatus>
+        assertEquals(XmlPullParser.END_TAG, parser.eventType)
+        assertEquals("multistatus", parser.name)
+    }
+
+    @Test
+    fun testParse_ValidProperty() {
+        val parser = XmlPullParserFactory.newInstance().apply {
+            isNamespaceAware = true
+        }.newPullParser()
+        parser.setInput(StringReader("<multistatus xmlns='DAV:'><getetag>12345</getetag></multistatus>"))
+        do {
+            parser.next()
+        } while (parser.eventType != XmlPullParser.START_TAG && parser.name != "multistatus")
+
+        // we're now at the start of <multistatus>
+        assertEquals(XmlPullParser.START_TAG, parser.eventType)
+        assertEquals("multistatus", parser.name)
+
+        val etag = Property.parse(parser).first()
+        assertEquals(GetETag("12345"), etag)
+
+        // we're now at the end of <multistatus>
+        assertEquals(XmlPullParser.END_TAG, parser.eventType)
+        assertEquals("multistatus", parser.name)
+    }
+
+}
diff --git a/src/test/kotlin/at/bitfire/dav4jvm/property/GetETagTest.kt b/src/test/kotlin/at/bitfire/dav4jvm/property/GetETagTest.kt
@@ -1,16 +1,14 @@
 package at.bitfire.dav4jvm.property
 
+import at.bitfire.dav4jvm.exception.InvalidPropertyException
 import org.junit.Assert.*
 import org.junit.Test
 
 class GetETagTest: PropertyTest() {
 
-    @Test
+    @Test()
     fun testGetETag_NoText() {
-        val results = parseProperty("<getetag xmlns=\"DAV:\"><invalid/></getetag>")
-        val getETag = results.first() as GetETag
-        assertNull(getETag.eTag)
-        assertNull(getETag.weak)
+        assertTrue(parseProperty("<getetag xmlns=\"DAV:\"><invalid/></getetag>").isEmpty())
     }
 
     @Test
