Sanitize user controlled arguments in bot comments

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."
  }
 })