SonarCube Blocker Violations - constructing the path from file name entry of an archive + missing assertions in tests

[igordayen]

There are a few Blockers reported by SonarCube that require attention:

https://sonarcloud.io/project/issues?impactSeverities=BLOCKER&issueStatuses=OPEN%2CCONFIRMED&id=embabel_embabel-agent

[simeshev]

The fix is trivial - replace using creating the file path directly from the zip entry name with a canonical path. Sonar has a suggestion. Can fix it in no time.

class Example {
    companion object {
        private const val TARGET_DIRECTORY = "/example/directory/"
    }
    fun extractEntry(zipFile: ZipFile) {
        val entries = zipFile.entries()
        val entry = entries.nextElement()
        val inputStream = zipFile.getInputStream(entry)
        val file = File(TARGET_DIRECTORY + entry.name)
        val canonicalDestinationPath = file.canonicalPath
        if (canonicalDestinationPath.startsWith(TARGET_DIRECTORY)) {
            inputStream.copyTo(file.outputStream())
        }
    }
}

[simeshev]

1. @johnsonr, Sonar complains about this code executing a command generated from the user input. This is the purpose of the method. Should I mark it as to be ignored by Sonar? The alternative is to enumerate all allowed commands. Thoughts?

Same true for CiTools.CiTools.buildProject and CiTools.buildProjectInteractive
2. BashTools are not used anywhere in the codebase. Should we just remove the class BashTools all together?

class BashTools(private val workingDirectory: String) {

    @Tool(description = "Generic bash tool")
    fun runBashCommand(command: String): String {
        val process = ProcessBuilder("/bin/bash", "-c", command)
            .directory(File(workingDirectory))
            .redirectErrorStream(true)
            .start()
        return process.inputStream.bufferedReader().readText()
    }
}

[simeshev]

@igordayen - any thougths?

[simeshev]

I've split this issue into two. One is this one the scope reduced to the constructing the path from file name entry of an archive + missing assertions in tests, and the new one is #1152 to handle the arbitrary command execution as they are unrelated and are touching different parts of the system.