Make various processors more defensive

CHANGELOG

@@ -1,7 +1,12 @@
 vNEXT (in development)
 
  * Fix issue parsing CTCPs when the content contained multi-byte chars
- * If the server provides an invalid timestamp, the event now defaults to the current time
+ * Fixed multiple issues when receiving malformed input from the server:
+   * Invalid server-time tags are now ignored and the current time used
+   * PRIVMSGs, NOTICEs and TOPICs with no target/message are now ignored
+   * CAPs with invalid or missing arguments are now ignored
+   * Better handling for duplicate JOIN messages
+   * Ignored messages are now logged more consistently
 
 v0.10.2
 

src/main/kotlin/com/dmdirc/ktirc/messages/processors/CapabilityProcessor.kt

@@ -14,10 +14,17 @@ internal class CapabilityProcessor : MessageProcessor {
 
     override val commands = arrayOf("CAP")
 
-    override fun process(message: IrcMessage) = when (message.subCommand) {
-        "LS" -> handleList(message.metadata, message.subCommandArguments)
-        "ACK" -> listOf(ServerCapabilitiesAcknowledged(message.metadata, message.params.capabilities))
-        else -> emptyList()
+    override fun process(message: IrcMessage) = when {
+        message.params.size < 2 -> {
+            log.warning { "Discarding CAP with insufficient args: $message" }
+            emptyList()
+        }
+        message.subCommand == "LS" -> handleList(message.metadata, message.subCommandArguments)
+        message.subCommand == "ACK" -> listOf(ServerCapabilitiesAcknowledged(message.metadata, message.params.capabilities))
+        else -> {
+            log.warning { "Discarding CAP with unknown subcommand: $message" }
+            emptyList()
+        }
     }
 
     private fun handleList(metadata: EventMetadata, lsParams: List<ByteArray>) = sequence {

src/main/kotlin/com/dmdirc/ktirc/messages/processors/ModeProcessor.kt

@@ -4,14 +4,23 @@ import com.dmdirc.ktirc.events.ModeChanged
 import com.dmdirc.ktirc.messages.RPL_CHANNELMODEIS
 import com.dmdirc.ktirc.messages.RPL_UMODEIS
 import com.dmdirc.ktirc.model.IrcMessage
+import com.dmdirc.ktirc.util.logger
 
 internal class ModeProcessor : MessageProcessor {
 
+    private val log by logger()
+
     override val commands = arrayOf(RPL_CHANNELMODEIS, RPL_UMODEIS, "MODE")
 
     override fun process(message: IrcMessage): List<ModeChanged> {
         val isDiscovery = message.command == RPL_CHANNELMODEIS || message.command == RPL_UMODEIS
         val paramOffset = if (message.command == RPL_CHANNELMODEIS) 1 else 0
+
+        if (message.params.size < paramOffset + 2) {
+            log.warning { "Discarding MODE line with insufficient parameters: $message" }
+            return emptyList()
+        }
+
         return listOf(ModeChanged(
                 message.metadata,
                 target = String(message.params[paramOffset]),

src/main/kotlin/com/dmdirc/ktirc/messages/processors/NoticeProcessor.kt

@@ -6,20 +6,28 @@ import com.dmdirc.ktirc.events.NoticeReceived
 import com.dmdirc.ktirc.messages.CTCP_BYTE
 import com.dmdirc.ktirc.model.IrcMessage
 import com.dmdirc.ktirc.model.User
+import com.dmdirc.ktirc.util.logger
 
 internal class NoticeProcessor : MessageProcessor {
 
+    private val log by logger()
+
     override val commands = arrayOf("NOTICE")
 
     override fun process(message: IrcMessage) = when {
-            message.isCtcp() -> handleCtcp(message,  message.sourceUser)
-            else -> listOf(NoticeReceived(message.metadata,  message.sourceUser ?: User("*"), String(message.params[0]), String(message.params[1])))
+        message.params.size < 2 -> {
+            log.warning { "Discarding NOTICE line with insufficient parameters: $message" }
+            emptyList()
         }
+        message.isCtcp() -> handleCtcp(message, message.sourceUser)
+        else -> listOf(NoticeReceived(message.metadata, message.sourceUser
+                ?: User("*"), String(message.params[0]), String(message.params[1])))
+    }
 
     private fun handleCtcp(message: IrcMessage, user: User?): List<IrcEvent> {
         user ?: return emptyList()
         val content = String(message.params[1].sliceArray(1 until message.params[1].size - 1))
-        val parts = content.split(' ', limit=2)
+        val parts = content.split(' ', limit = 2)
         val body = if (parts.size == 2) parts[1] else ""
         return listOf(CtcpReplyReceived(message.metadata, user, String(message.params[0]), parts[0], body))
     }

src/main/kotlin/com/dmdirc/ktirc/messages/processors/PrivmsgProcessor.kt

@@ -7,21 +7,27 @@ import com.dmdirc.ktirc.events.MessageReceived
 import com.dmdirc.ktirc.messages.CTCP_BYTE
 import com.dmdirc.ktirc.model.IrcMessage
 import com.dmdirc.ktirc.model.User
+import com.dmdirc.ktirc.util.logger
 
 internal class PrivmsgProcessor : MessageProcessor {
 
+    private val log by logger()
+
     override val commands = arrayOf("PRIVMSG")
 
-    override fun process(message: IrcMessage) = message.sourceUser?.let { user ->
-        listOf(when {
-            message.isCtcp() -> handleCtcp(message, user)
-            else -> MessageReceived(message.metadata, user, String(message.params[0]), String(message.params[1]))
-        })
-    } ?: emptyList()
+    override fun process(message: IrcMessage) = when {
+        message.sourceUser == null -> emptyList()
+        message.params.size < 2 -> {
+            log.warning { "Discarding PRIVMSG line with insufficient parameters: $message" }
+            emptyList()
+        }
+        message.isCtcp() -> listOf(handleCtcp(message, message.sourceUser))
+        else -> listOf(MessageReceived(message.metadata, message.sourceUser, String(message.params[0]), String(message.params[1])))
+    }
 
     private fun handleCtcp(message: IrcMessage, user: User): IrcEvent {
         val content = String(message.params[1].sliceArray(1 until message.params[1].size - 1))
-        val parts = content.split(' ', limit=2)
+        val parts = content.split(' ', limit = 2)
         val body = if (parts.size == 2) parts[1] else ""
         return when (parts[0].toUpperCase()) {
             "ACTION" -> ActionReceived(message.metadata, user, String(message.params[0]), body)

src/main/kotlin/com/dmdirc/ktirc/messages/processors/TopicProcessor.kt

@@ -9,11 +9,14 @@ import com.dmdirc.ktirc.messages.RPL_TOPICWHOTIME
 import com.dmdirc.ktirc.model.IrcMessage
 import com.dmdirc.ktirc.model.asUser
 import com.dmdirc.ktirc.util.currentTimeZoneProvider
+import com.dmdirc.ktirc.util.logger
 import java.time.Instant
 import java.time.LocalDateTime
 
 internal class TopicProcessor : MessageProcessor {
 
+    private val log by logger()
+
     override val commands = arrayOf(RPL_TOPIC, RPL_TOPICWHOTIME, RPL_NOTOPIC, "TOPIC")
 
     override fun process(message: IrcMessage) = sequence {
@@ -22,7 +25,13 @@ internal class TopicProcessor : MessageProcessor {
             RPL_NOTOPIC -> yield(ChannelTopicDiscovered(message.metadata, message.channel, null))
             RPL_TOPICWHOTIME -> yield(ChannelTopicMetadataDiscovered(
                     message.metadata, message.channel, message.params[2].asUser(), message.topicSetTime))
-            "TOPIC" -> message.sourceUser?.let { yield(ChannelTopicChanged(message.metadata, it, String(message.params[0]), String(message.params[1]))) }
+            "TOPIC" -> message.sourceUser?.let {
+                if (message.params.size >= 2) {
+                    yield(ChannelTopicChanged(message.metadata, it, String(message.params[0]), String(message.params[1])))
+                } else {
+                    log.warning { "Discarding TOPIC line with insufficient parameters: $message" }
+                }
+            }
         }
     }.toList()
 

src/main/kotlin/com/dmdirc/ktirc/messages/processors/WelcomeProcessor.kt

@@ -8,8 +8,12 @@ internal class WelcomeProcessor : MessageProcessor {
 
     override val commands = arrayOf(RPL_WELCOME)
 
-    override fun process(message: IrcMessage) = listOf(ServerWelcome(message.metadata, message.serverName(), String(message.params[0])))
+    override fun process(message: IrcMessage) = listOf(ServerWelcome(
+            message.metadata,
+            message.serverName(),
+            message.localNick()))
 
     private fun IrcMessage.serverName() = prefix?.let { String(it) } ?: ""
+    private fun IrcMessage.localNick() = if (params.isEmpty()) "" else String(params[0])
 
 }

src/main/kotlin/com/dmdirc/ktirc/model/IrcMessage.kt

@@ -23,19 +23,21 @@ internal class IrcMessage(val tags: Map<MessageTag, String>, val prefix: ByteArr
             label = tags[MessageTag.Label])
 
     /** The user that generated the message, if any. */
-    val sourceUser by lazy {
-        prefix?.asUser()?.apply {
-            tags[MessageTag.AccountName]?.let { account = it }
-        }
+    val sourceUser = prefix?.asUser()?.apply {
+        tags[MessageTag.AccountName]?.let { account = it }
     }
 
     private fun String.toLocalDateOrNull() = try {
         LocalDateTime.ofInstant(Instant.parse(this), currentTimeZoneProvider())
-    } catch(e: Exception) {
+    } catch (e: Exception) {
         log.log(Level.WARNING, e) { "Received unparsable server-time tag: $this" }
         null
     }
 
+    override fun toString(): String {
+        return "IrcMessage(tags=$tags, prefix=${prefix?.let { String(prefix) }}, command='$command', params=${params.map { String(it) }}, metadata=$metadata, sourceUser=$sourceUser)"
+    }
+
 }
 
 /**

src/main/kotlin/com/dmdirc/ktirc/model/Maps.kt

@@ -14,7 +14,6 @@ abstract class CaseInsensitiveMap<T>(private val caseMappingProvider: () -> Case
     operator fun get(name: String) = synchronized(values) { values.find { caseMappingProvider().areEquivalent(nameOf(it), name) } }
 
     internal operator fun plusAssign(value: T): Unit = synchronized(values) {
-        require(get(nameOf(value)) == null) { "Value already registered: ${nameOf(value)}" }
         values.add(value)
     }
 

src/test/kotlin/com/dmdirc/ktirc/events/handlers/ChannelStateHandlerTest.kt

@@ -47,6 +47,17 @@ internal class ChannelStateHandlerTest {
         assertTrue("zerocool" in fakeChannelState["#thegibson"]?.users!!)
     }
 
+    @Test
+    fun `ignores duplicate joiners`() {
+        fakeChannelState += ChannelState("#thegibson") { CaseMapping.Rfc }
+
+        handler.processEvent(ircClient, ChannelJoined(EventMetadata(TestConstants.time), User("zerocool", "dade", "root.localhost"), "#thegibson"))
+        handler.processEvent(ircClient, ChannelJoined(EventMetadata(TestConstants.time), User("zerocool", "dade", "root.localhost"), "#thegibson"))
+
+        assertTrue("zerocool" in fakeChannelState["#thegibson"]?.users!!)
+        assertEquals(1, fakeChannelState["#thegibson"]?.users?.count())
+    }
+
     @Test
     fun `clears existing users when getting a new list`() {
         val channel = ChannelState("#thegibson") { CaseMapping.Rfc }

src/test/kotlin/com/dmdirc/ktirc/messages/processors/CapabilityProcessorTest.kt

@@ -14,14 +14,21 @@ internal class CapabilityProcessorTest {
     private val processor = CapabilityProcessor()
 
     @Test
-    fun `CapabilityProcessor does nothing for unknown subcommand`() {
+    fun `does nothing for unknown subcommand`() {
         val events = processor.process(IrcMessage(emptyMap(), "the.gibson".toByteArray(), "CAP", params("*", "FOO")))
 
         assertTrue(events.isEmpty())
     }
 
     @Test
-    fun `CapabilityProcessor raises ServerCapabilitiesReceived event given no capabilities`() {
+    fun `does nothing for missing subcommand`() {
+        val events = processor.process(IrcMessage(emptyMap(), "the.gibson".toByteArray(), "CAP", params("*")))
+
+        assertTrue(events.isEmpty())
+    }
+
+    @Test
+    fun `raises ServerCapabilitiesReceived event given no capabilities`() {
         val events = processor.process(IrcMessage(emptyMap(), "the.gibson".toByteArray(), "CAP", params("*", "LS", "")))
 
         val receivedEvent = events.filterIsInstance<ServerCapabilitiesReceived>()[0]
@@ -30,7 +37,7 @@ internal class CapabilityProcessorTest {
 
 
     @Test
-    fun `CapabilityProcessor raises ServerCapabilitiesReceived event with known capabilities`() {
+    fun `raises ServerCapabilitiesReceived event with known capabilities`() {
         val events = processor.process(IrcMessage(emptyMap(), "the.gibson".toByteArray(), "CAP", params("*", "LS", "chghost extended-join invalid")))
 
         val receivedEvent = events.filterIsInstance<ServerCapabilitiesReceived>()[0]
@@ -40,7 +47,7 @@ internal class CapabilityProcessorTest {
     }
 
     @Test
-    fun `CapabilityProcessor raises ServerCapabilitiesReceived event with values for capabilities`() {
+    fun `raises ServerCapabilitiesReceived event with values for capabilities`() {
         val events = processor.process(IrcMessage(emptyMap(), "the.gibson".toByteArray(), "CAP", params("*", "LS", "chghost=test123 extended-join=abc=def invalid")))
 
         val receivedEvent = events.filterIsInstance<ServerCapabilitiesReceived>()[0]
@@ -50,7 +57,7 @@ internal class CapabilityProcessorTest {
     }
 
     @Test
-    fun `CapabilityProcessor overwrites earlier values with later ones for identical capabilities`() {
+    fun `overwrites earlier values with later ones for identical capabilities`() {
         val events = processor.process(IrcMessage(emptyMap(), "the.gibson".toByteArray(), "CAP", params("*", "LS", "chghost=test123 chghost chghost=456")))
 
         val receivedEvent = events.filterIsInstance<ServerCapabilitiesReceived>()[0]
@@ -60,7 +67,7 @@ internal class CapabilityProcessorTest {
 
 
     @Test
-    fun `CapabilityProcessor raises ServerCapabilitiesReceived event with known capabilities for multi-line responses`() {
+    fun `raises ServerCapabilitiesReceived event with known capabilities for multi-line responses`() {
         val events = processor.process(IrcMessage(emptyMap(), "the.gibson".toByteArray(), "CAP", params("*", "LS", "*", "chghost extended-join invalid")))
 
         val receivedEvent = events.filterIsInstance<ServerCapabilitiesReceived>()[0]
@@ -70,7 +77,7 @@ internal class CapabilityProcessorTest {
     }
 
     @Test
-    fun `CapabilityProcessor raises ServerCapabilitiesFinished event for final LS responses`() {
+    fun `raises ServerCapabilitiesFinished event for final LS responses`() {
         val events = processor.process(IrcMessage(emptyMap(), "the.gibson".toByteArray(), "CAP", params("*", "LS", "chghost extended-join invalid")))
 
         assertEquals(2, events.size)
@@ -78,7 +85,7 @@ internal class CapabilityProcessorTest {
     }
 
     @Test
-    fun `CapabilityProcessor raises ServerCapabilitiesAcknowledged event`() {
+    fun `raises ServerCapabilitiesAcknowledged event`() {
         val events = processor.process(IrcMessage(emptyMap(), "the.gibson".toByteArray(), "CAP", params("*", "ACK", "chghost=test123 extended-join=abc=def invalid")))
 
         val receivedEvent = events.filterIsInstance<ServerCapabilitiesAcknowledged>()[0]

src/test/kotlin/com/dmdirc/ktirc/messages/processors/ModeProcessorTest.kt

@@ -127,4 +127,11 @@ internal class ModeProcessorTest {
         assertFalse(events[0].discovered)
     }
 
+    @Test
+    fun `ignores mode changes with a missing target`() {
+        val events = ModeProcessor().process(
+                IrcMessage(emptyMap(), "acidburn!libby@root.localhost".toByteArray(), "MODE", params("+hax")))
+        assertEquals(0, events.size)
+    }
+
 }

src/test/kotlin/com/dmdirc/ktirc/messages/processors/NoticeProcessorTest.kt

@@ -92,4 +92,11 @@ internal class NoticeProcessorTest {
                 IrcMessage(emptyMap(), null, "NOTICE", params("#crashandburn", "\u0001BORK\u0001")))
         assertEquals(0, events.size)
     }
+
+    @Test
+    fun `does nothing if target missing for CTCP replies`() {
+        val events = NoticeProcessor().process(
+                IrcMessage(emptyMap(), "acidburn!libby@root.localhost".toByteArray(), "NOTICE", params("\u0001BORK\u0001")))
+        assertEquals(0, events.size)
+    }
 }

src/test/kotlin/com/dmdirc/ktirc/messages/processors/PrivmsgProcessorTest.kt

@@ -119,4 +119,11 @@ internal class PrivmsgProcessorTest {
                 IrcMessage(emptyMap(), null, "PRIVMSG", params("#crashandburn", "hack the planet!")))
         assertEquals(0, events.size)
     }
+
+    @Test
+    fun `does nothing if arguments missing`() {
+        val events = PrivmsgProcessor().process(
+                IrcMessage(emptyMap(), "acidburn!libby@root.localhost".toByteArray(), "PRIVMSG", params("hack the planet!")))
+        assertEquals(0, events.size)
+    }
 }

src/test/kotlin/com/dmdirc/ktirc/messages/processors/TopicProcessorTest.kt

@@ -75,6 +75,12 @@ internal class TopicProcessorTest {
         assertEquals(0, events.size)
     }
 
+    @Test
+    fun `does nothing when topic is changed with no channel`() {
+        val events = processor.process(IrcMessage(emptyMap(), "acidBurn!acidB@the.gibson".toByteArray(), "TOPIC", params("Hack the planet!")))
+        assertEquals(0, events.size)
+    }
+
     private fun unixtime() = TestConstants.otherTime.toEpochSecond(currentTimeZoneProvider().rules.getOffset(TestConstants.time)).toString()
 
 }

src/test/kotlin/com/dmdirc/ktirc/messages/processors/WelcomeProcessorTest.kt

@@ -18,7 +18,7 @@ internal class WelcomeProcessorTest {
     }
 
     @Test
-    fun `WelcomeProcessor returns server welcome event`() {
+    fun `returns server welcome event`() {
         val events = processor.process(IrcMessage(emptyMap(), "thegibson.com".toByteArray(), "001", params(
                 "acidBurn", "Welcome to the Internet Relay Network, acidBurn!burn@hacktheplanet.com")))
         assertEquals(1, events.size)
@@ -27,4 +27,23 @@ internal class WelcomeProcessorTest {
         assertEquals("thegibson.com", events[0].server)
     }
 
+    @Test
+    fun `returns blank server if no prefix is specified`() {
+        val events = processor.process(IrcMessage(emptyMap(), null, "001", params(
+                "acidBurn", "Welcome to the Internet Relay Network, acidBurn!burn@hacktheplanet.com")))
+        assertEquals(1, events.size)
+        assertEquals(TestConstants.time, events[0].metadata.time)
+        assertEquals("acidBurn", events[0].localNick)
+        assertEquals("", events[0].server)
+    }
+
+    @Test
+    fun `returns blank local nickname if no parameters provided`() {
+        val events = processor.process(IrcMessage(emptyMap(), "thegibson.com".toByteArray(), "001", params()))
+        assertEquals(1, events.size)
+        assertEquals(TestConstants.time, events[0].metadata.time)
+        assertEquals("", events[0].localNick)
+        assertEquals("thegibson.com", events[0].server)
+    }
+
 }

src/test/kotlin/com/dmdirc/ktirc/model/MapsTest.kt

@@ -3,7 +3,6 @@ package com.dmdirc.ktirc.model
 import com.dmdirc.ktirc.io.CaseMapping
 import org.junit.jupiter.api.Assertions.*
 import org.junit.jupiter.api.Test
-import org.junit.jupiter.api.assertThrows
 
 internal class CaseInsensitiveMapTest {
 
@@ -13,7 +12,7 @@ internal class CaseInsensitiveMapTest {
     private val map = object : CaseInsensitiveMap<Wrapper>({ caseMapping }, { it -> it.name }) {}
 
     @Test
-    fun `CaseInsensitiveMap stores values`() {
+    fun `stores values`() {
         val value = Wrapper("acidBurn")
 
         map += value
@@ -22,18 +21,18 @@ internal class CaseInsensitiveMapTest {
     }
 
     @Test
-    fun `CaseInsensitiveMap disallows the same value twice`() {
+    fun `ignores the same value twice`() {
         val value = Wrapper("acidBurn")
 
+        map += value
         map += value
 
-        assertThrows<IllegalArgumentException> {
-            map += value
-        }
+        assertSame(value, map["acidBurn"])
+        assertEquals(1, map.count())
     }
 
     @Test
-    fun `CaseInsensitiveMap retrieves values using differently cased keys`() {
+    fun `retrieves values using differently cased keys`() {
         val value = Wrapper("[acidBurn]")
         map += value