Sanitize user controlled arguments in bot comments (#732)

* Sanitize user controlled arguments in bot comments

* Make CommandModule ignore comments written by bot

src/main/kotlin/io/github/mojira/arisa/infrastructure/HelperMessages.kt

@@ -6,6 +6,7 @@ import arrow.core.right
 import arrow.core.rightIfNotNull
 import com.beust.klaxon.Klaxon
 import com.beust.klaxon.KlaxonException
+import io.github.mojira.arisa.infrastructure.jira.sanitizeCommentArg
 import io.github.mojira.arisa.modules.openHttpGetInputStream
 import org.slf4j.LoggerFactory
 import java.io.File
@@ -83,7 +84,7 @@ object HelperMessageService {
         return data.messages[key]?.find { isProjectMatch(project, it.project) }
             .rightIfNotNull { Error("Failed to find message for key $key under project $project") }
             .map { localizeValue(it.message, it.localizedMessages, lang) }
-            .map { resolvePlaceholder(it, filledText) }
+            .map { resolvePlaceholder(it, filledText?.let(::sanitizeCommentArg)) }
             .map { resolveVariables(it, project, lang) }
     }
 

src/main/kotlin/io/github/mojira/arisa/infrastructure/jira/Functions.kt

@@ -408,3 +408,10 @@ fun markAsFixedWithSpecificVersion(context: Lazy<IssueUpdateContext>, fixVersion
 fun changeReporter(context: Lazy<IssueUpdateContext>, reporter: String) {
     context.value.update.field(Field.REPORTER, reporter)
 }
+
+// Allows some basic Jira formatting characters to be used by helper message arguments;
+// when used by malicious user they should at most cause text formatting errors
+private val sanitizationRegex = Regex("[^a-zA-Z0-9\\-+_#*?.,; ]")
+fun sanitizeCommentArg(arg: String): String {
+    return arg.replace(sanitizationRegex, "?")
+}

src/main/kotlin/io/github/mojira/arisa/modules/AttachmentModule.kt

@@ -6,6 +6,7 @@ import arrow.syntax.function.partially1
 import io.github.mojira.arisa.domain.Attachment
 import io.github.mojira.arisa.domain.CommentOptions
 import io.github.mojira.arisa.domain.Issue
+import io.github.mojira.arisa.infrastructure.jira.sanitizeCommentArg
 import java.time.Instant
 
 class AttachmentModule(
@@ -28,9 +29,11 @@ class AttachmentModule(
         }
     }
 
-    private fun List<Attachment>.getCommentInfo() = this
-        .map { "- [~${it.uploader!!.name}]: ${it.name}" }
-        .joinToString(separator = "\n")
+    private fun List<Attachment>.getCommentInfo() = this.joinToString(separator = "\n") {
+        val uploaderName = it.uploader!!.name?.let(::sanitizeCommentArg)
+        val attachmentName = sanitizeCommentArg(it.name)
+        "- [~$uploaderName]: $attachmentName"
+    }
 
     private fun endsWithBlacklistedExtensions(extensionBlackList: List<String>, name: String) =
         extensionBlackList.any { name.endsWith(it) }

src/main/kotlin/io/github/mojira/arisa/modules/CommandModule.kt

@@ -46,9 +46,7 @@ class CommandModule(
                 }
                 .filter { it.second.isNotEmpty() }
                 .onEach { invocation ->
-                    val commandResults = invocation.second
-                        .map { it.source.line to executeCommand(it) }
-                        .toMap()
+                    val commandResults = invocation.second.associate { it.source.line to executeCommand(it) }
                     editInvocationComment(invocation.first, commandResults)
                 }
             assertNotEmpty(commands).bind()
@@ -119,8 +117,15 @@ class CommandModule(
         """.trimMargin()
     }
 
-    private fun userIsVolunteer(comment: Comment) =
-        comment.getAuthorGroups()?.any { it == "helper" || it == "global-moderators" || it == "staff" } ?: false
+    private fun userIsVolunteer(comment: Comment): Boolean {
+        // Ignore comments from the bot itself to prevent accidental infinite recursion and command
+        // injection by malicious user
+        if (comment.author.name == botUserName) {
+            return false
+        }
+
+        return comment.getAuthorGroups()?.any { it == "helper" || it == "global-moderators" || it == "staff" } ?: false
+    }
 
     private fun isStaffRestricted(comment: Comment) =
         comment.visibilityType == "group" && (comment.visibilityValue == "staff" || comment.visibilityValue == "helper")

src/main/kotlin/io/github/mojira/arisa/modules/commands/ListUserActivityCommand.kt

@@ -2,6 +2,7 @@ package io.github.mojira.arisa.modules.commands
 
 import arrow.core.Either
 import io.github.mojira.arisa.domain.Issue
+import io.github.mojira.arisa.infrastructure.jira.sanitizeCommentArg
 
 /**
  * How many tickets should be listed at max.
@@ -20,19 +21,22 @@ class ListUserActivityCommand(
             | OR issueFunction IN fileAttached("by '$escapedUserName'")"""
             .trimMargin().replace("[\n\r]", "")
 
+        val sanitizedUserName = sanitizeCommentArg(userName)
+
         val tickets = when (val either = searchIssues(jql, ACTIVITY_LIST_CAP)) {
-            is Either.Left -> throw CommandExceptions.CANNOT_QUERY_USER_ACTIVITY.create(userName)
+            is Either.Left -> throw CommandExceptions.CANNOT_QUERY_USER_ACTIVITY.create(sanitizedUserName)
             is Either.Right -> either.b
         }
 
         if (tickets.isNotEmpty()) {
             issue.addRawRestrictedComment(
-                "User \"$userName\" left comments on the following tickets:\n* ${tickets.joinToString("\n* ")}",
+                "User \"$sanitizedUserName\" left comments on the following tickets:" +
+                    "\n* ${tickets.joinToString("\n* ")}",
                 "staff"
             )
         } else {
             issue.addRawRestrictedComment(
-                """No unrestricted comments from user "$userName" were found.""",
+                """No unrestricted comments from user "$sanitizedUserName" were found.""",
                 "staff"
             )
         }

src/main/kotlin/io/github/mojira/arisa/modules/commands/RemoveContentCommand.kt

@@ -2,6 +2,7 @@ package io.github.mojira.arisa.modules.commands
 
 import arrow.core.Either
 import io.github.mojira.arisa.infrastructure.escapeIssueFunction
+import io.github.mojira.arisa.infrastructure.jira.sanitizeCommentArg
 import io.github.mojira.arisa.log
 import net.rcarz.jiraclient.Attachment
 import net.rcarz.jiraclient.Comment
@@ -70,7 +71,7 @@ class RemoveContentCommand(
             .trimMargin().replace("[\n\r]", "")
 
         val ticketIds = when (val either = searchIssues(jql, REMOVABLE_ACTIVITY_CAP)) {
-            is Either.Left -> throw CommandExceptions.CANNOT_QUERY_USER_ACTIVITY.create(userName)
+            is Either.Left -> throw CommandExceptions.CANNOT_QUERY_USER_ACTIVITY.create(sanitizeCommentArg(userName))
             is Either.Right -> either.b
         }
 
@@ -79,7 +80,7 @@ class RemoveContentCommand(
 
             issue.addRawRestrictedComment(
                 "Removed ${result.removedComments} comments " +
-                        "and ${result.removedAttachments} attachments from user \"$userName\".",
+                    "and ${result.removedAttachments} attachments from user \"${sanitizeCommentArg(userName)}\".",
                 "staff"
             )
         }

src/test/kotlin/io/github/mojira/arisa/infrastructure/HelperMessagesTest.kt

@@ -198,6 +198,22 @@ class HelperMessagesTest : StringSpec({
         result.b shouldBe "With MC-4"
     }
 
+    "should sanitize filled text" {
+        val maliciousFilledText = "bad\n\r\n\u0000\"'\u202E"
+        val result = HelperMessageService.getSingleMessage("MC", "with-placeholder", maliciousFilledText)
+
+        result.shouldBeRight()
+        result.b shouldBe "With bad???????"
+    }
+
+    "should allow basic formatting for filled text" {
+        val filledText = "test *a* and _b_"
+        val result = HelperMessageService.getSingleMessage("MC", "with-placeholder", filledText)
+
+        result.shouldBeRight()
+        result.b shouldBe "With test *a* and _b_"
+    }
+
     "should use the original value when the lang doesn't exist" {
         val result = HelperMessageService.getSingleMessage("MC", "normal", lang = "cd")
 

src/test/kotlin/io/github/mojira/arisa/modules/AttachmentModuleTest.kt

@@ -3,11 +3,13 @@ package io.github.mojira.arisa.modules
 import io.github.mojira.arisa.domain.CommentOptions
 import io.github.mojira.arisa.utils.mockAttachment
 import io.github.mojira.arisa.utils.mockIssue
+import io.github.mojira.arisa.utils.mockUser
 import io.kotest.assertions.arrow.either.shouldBeLeft
 import io.kotest.assertions.arrow.either.shouldBeRight
 import io.kotest.core.spec.style.StringSpec
 import io.kotest.matchers.booleans.shouldBeTrue
 import io.kotest.matchers.shouldBe
+import io.kotest.matchers.string.shouldContain
 import java.time.Instant
 
 private val NOW = Instant.now()
@@ -57,31 +59,43 @@ class AttachmentModuleTest : StringSpec({
 
     "should comment with attachment details when an attachment is removed" {
         var removedAttachment = false
-        var attachmentContent = ""
+        var comment = ""
+        var commentRestriction: String? = null
         val module = AttachmentModule(listOf(".test"), "attach-new-attachment")
         val attachment = getAttachment(
+            name = "evil\nAttachment.test",
+            uploaderName = "evil\nUser",
             remove = { removedAttachment = true }
         )
         val issue = mockIssue(
             attachments = listOf(attachment),
-            addRawRestrictedComment = { it, _ -> attachmentContent = it }
+            addRawRestrictedComment = { body, restriction ->
+                comment = body
+                commentRestriction = restriction
+            }
         )
 
         val result = module(issue, NOW)
 
         result.shouldBeRight(ModuleResponse)
         removedAttachment.shouldBeTrue()
-        attachmentContent.contains(".test").shouldBeTrue()
+        commentRestriction shouldBe "helper"
+        // Should contain sanitized user name
+        comment shouldContain "evil?User"
+        // Should contain sanitized attachment name
+        comment shouldContain "evil?Attachment"
     }
 })
 
 private fun getAttachment(
     name: String = "testfile.test",
     created: Instant = NOW,
+    uploaderName: String = "someUser",
     remove: () -> Unit = { }
 ) = mockAttachment(
     name = name,
     created = created,
+    uploader = mockUser(name = uploaderName),
     remove = remove,
     getContent = { ByteArray(0) }
 )

src/test/kotlin/io/github/mojira/arisa/modules/CommandModuleTest.kt

@@ -58,7 +58,7 @@ class CommandModuleTest : StringSpec({
         result.shouldBeLeft(OperationNotNeededModuleResponse)
     }
 
-    "should return OperationNotNeededModuleResponse when comment doesnt have correct group" {
+    "should return OperationNotNeededModuleResponse when comment doesn't have correct group" {
         val module = CommandModule("ARISA", "userName", ::getDispatcher)
         val comment = getComment(
             visibilityType = "notagroup"
@@ -72,7 +72,7 @@ class CommandModuleTest : StringSpec({
         result.shouldBeLeft(OperationNotNeededModuleResponse)
     }
 
-    "should return OperationNotNeededModuleResponse when comment doesnt have correct value" {
+    "should return OperationNotNeededModuleResponse when comment doesn't have correct value" {
         val module = CommandModule("ARISA", "userName", ::getDispatcher)
         val comment = getComment(
             visibilityValue = "notagroup"
@@ -101,7 +101,7 @@ class CommandModuleTest : StringSpec({
         result.shouldBeLeft(OperationNotNeededModuleResponse)
     }
 
-    "should return OperationNotNeededModuleResponse when comment doesnt start with ARISA_" {
+    "should return OperationNotNeededModuleResponse when comment doesn't start with ARISA_" {
         val module = CommandModule("ARISA", "userName", ::getDispatcher)
         val comment = getComment(
             body = "ARISA"
@@ -115,6 +115,23 @@ class CommandModuleTest : StringSpec({
         result.shouldBeLeft(OperationNotNeededModuleResponse)
     }
 
+    "should return OperationNotNeededModuleResponse when comment has been written by bot" {
+        val botName = "botName"
+        val module = CommandModule("ARISA", botName, ::getDispatcher)
+        val comment = getComment(
+            author = mockUser(
+                name = botName
+            )
+        )
+        val issue = mockIssue(
+            comments = listOf(comment)
+        )
+
+        val result = module(issue, RIGHT_NOW)
+
+        result.shouldBeLeft(OperationNotNeededModuleResponse)
+    }
+
     "should return successfully when comment matches a command and it returns successfully" {
         var updatedComment = ""
         val module = CommandModule("ARISA", "userName", ::getDispatcher)

src/test/kotlin/io/github/mojira/arisa/modules/commands/ListUserActivityCommandTest.kt

@@ -13,7 +13,7 @@ class ListUserActivityCommandTest : StringSpec({
     "should query user activity and post a comment with all tickets" {
         var calledSearch = false
         var comment: String? = null
-        var commentRestrictions: String? = null
+        var commentRestriction: String? = null
 
         val issueList = listOf("MC-1", "MC-12", "MC-1234", "MC-12345")
 
@@ -25,21 +25,53 @@ class ListUserActivityCommandTest : StringSpec({
         }
 
         val issue = mockIssue(
-            addRawRestrictedComment = { body, restrictions ->
+            addRawRestrictedComment = { body, restriction ->
                 comment = body
-                commentRestrictions = restrictions
+                commentRestriction = restriction
             }
         )
 
-        val result = command(issue, "userName")
+        val result = command(issue, "user\nName")
 
         result shouldBe 1
         calledSearch.shouldBeTrue()
         comment.shouldNotBeNull()
-        commentRestrictions.shouldBe("staff")
+        commentRestriction shouldBe "staff"
 
         issueList.forEach {
-            comment.shouldContain(it)
+            comment shouldContain it
         }
+        // Should contain sanitized user name
+        comment shouldContain "user?Name"
+    }
+
+    "should post comment when no tickets were found" {
+        var calledSearch = false
+        var comment: String? = null
+        var commentRestriction: String? = null
+
+        val command = ListUserActivityCommand { _, _ ->
+            Either.fx {
+                calledSearch = true
+                emptyList()
+            }
+        }
+
+        val issue = mockIssue(
+            addRawRestrictedComment = { body, restriction ->
+                comment = body
+                commentRestriction = restriction
+            }
+        )
+
+        val result = command(issue, "user\nName")
+
+        result shouldBe 1
+        calledSearch.shouldBeTrue()
+        comment.shouldNotBeNull()
+        commentRestriction shouldBe "staff"
+
+        // Should contain sanitized user name
+        comment shouldBe "No unrestricted comments from user \"user?Name\" were found."
     }
 })