From 2a95b07b54b27fccb89e94146575c7de8c400854 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0est=C3=A1k=20V=C3=ADt?= Date: Wed, 11 Oct 2017 16:54:25 +0200 Subject: [PATCH] Added more fail safety for vulnerability export. This should affect all exports when a vulnerability disappears. --- app/controllers/Notifications.scala | 6 +-- app/models/vulnerabilityOverviews.scala | 49 ++++++++++++++++++++++ app/services/EmailExportService.scala | 49 +++------------------- app/services/IssueTrackerService.scala | 4 +- app/services/JiraIssueTrackerService.scala | 20 ++++----- app/services/OdcDbService.scala | 3 ++ 6 files changed, 72 insertions(+), 59 deletions(-) create mode 100644 app/models/vulnerabilityOverviews.scala diff --git a/app/controllers/Notifications.scala b/app/controllers/Notifications.scala index e14afff..c90a739 100644 --- a/app/controllers/Notifications.scala +++ b/app/controllers/Notifications.scala @@ -6,7 +6,7 @@ import javax.inject.Inject import com.ysoft.concurrent.FutureLock._ import com.ysoft.odc.statistics.{FailedProjects, LibDepStatistics} import com.ysoft.odc.{Absolutizer, ArtifactFile, ArtifactItem, SetDiff} -import models.{EmailMessageId, ExportedVulnerability} +import models.{EmailMessageId, ExportedVulnerability, StandardVulnerabilityOverview, VulnerabilityOverview} import modules.TemplateCustomization import play.api.i18n.MessagesApi import play.api.libs.Crypto @@ -59,7 +59,7 @@ class Notifications @Inject()( )( reportVulnerability: (Vulnerability, Set[GroupedDependency]) => Future[ExportedVulnerability[T]] )( - reportChangedProjectsForVulnerability: (Vulnerability, SetDiff[String], T) => Future[Unit] + reportChangedProjectsForVulnerability: (VulnerabilityOverview, SetDiff[String], T) => Future[Unit] ) = { val vulnerabilitiesByName = lds.vulnerabilitiesToDependencies.map{case (v, deps) => (v.name, (v, deps))} for{ @@ -82,7 +82,7 @@ class Notifications @Inject()( if(diff.nonEmpty) { for{ // Try to load vuln from memory; If the vuln has disappeared, we have to load it from DB. - vulnerability <- lds.vulnerabilitiesByName.get(vulnerabilityName).fold(odcService.getVulnerabilityDetails(vulnerabilityName).map(_.get))(Future(_)) + vulnerability <- lds.vulnerabilitiesByName.get(vulnerabilityName).fold(odcService.getVulnerabilityDescription(vulnerabilityName))(x => Future(new StandardVulnerabilityOverview(x))) (_: Unit) <- reportChangedProjectsForVulnerability(vulnerability, diff, exportedVulnerability.ticket) (_: Unit) <- ep.changeProjects(ticketId, diff, projects) } yield Some(diff) diff --git a/app/models/vulnerabilityOverviews.scala b/app/models/vulnerabilityOverviews.scala new file mode 100644 index 0000000..77c3042 --- /dev/null +++ b/app/models/vulnerabilityOverviews.scala @@ -0,0 +1,49 @@ +package models + +import com.ysoft.odc.CWE +import controllers.Vulnerability + +/** + * Provides some overview about vulnerability. It might be either covered by fully-detailed vulnerability or represent a vulnerability we know little or nothing about. + */ +abstract sealed class VulnerabilityOverview { + def name: String + def descriptionAttempt: String + def isSureAboutDescription: Boolean + def cvssScore: Option[Double] + def cweOption: Option[CWE] +} + +object VulnerabilityOverview{ + def apply(name: String, v: Option[Vulnerability]): VulnerabilityOverview = v.fold(UnknownVulnerabilityOverview(name))(new StandardVulnerabilityOverview(_)) +} + +final class StandardVulnerabilityOverview(vulnerability: Vulnerability) extends VulnerabilityOverview { + override def name: String = vulnerability.name + override def descriptionAttempt: String = vulnerability.description + override def isSureAboutDescription = true + override def cvssScore: Option[Double] = vulnerability.cvssScore + override def cweOption = vulnerability.cweOption +} + +private final class UnknownVulnerabilityOverview(override val name: String, link: String) extends VulnerabilityOverview { + override def descriptionAttempt: String = s"Unknown vulnerability. Try looking at the following address for more details: $link" + override def cvssScore: Option[Double] = None + override def isSureAboutDescription = false + override def cweOption = None +} + +private final class TotallyUnknownVulnerabilityOverview(override val name: String) extends VulnerabilityOverview { + override def descriptionAttempt: String = s"Unknown vulnerability. Not even sure where to look for other details. Maybe Googling the identifier will help." + override def cvssScore: Option[Double] = None + override def isSureAboutDescription = false + override def cweOption = None +} + +private object UnknownVulnerabilityOverview { + def apply(name: String): VulnerabilityOverview = name match { + case cveId if name startsWith "CVE-" => new UnknownVulnerabilityOverview(name, s"https://nvd.nist.gov/vuln/detail/$cveId") + case ossIndexId if name startsWith "OSSINDEX-" => new UnknownVulnerabilityOverview(name, s"https://ossindex.net/resource/vulnerability/$ossIndexId") + case other => new TotallyUnknownVulnerabilityOverview(other) + } +} diff --git a/app/services/EmailExportService.scala b/app/services/EmailExportService.scala index 9b5555c..d7aa320 100644 --- a/app/services/EmailExportService.scala +++ b/app/services/EmailExportService.scala @@ -8,7 +8,7 @@ import com.ysoft.html.HtmlWithText._ import com.ysoft.odc.{Absolutizer, SetDiff} import controllers._ import models.Change.Direction -import models.{Change, EmailMessageId} +import models.{Change, EmailMessageId, VulnerabilityOverview} import play.api.libs.mailer.{Email, MailerClient} import play.twirl.api.{Html, HtmlFormat} @@ -19,50 +19,11 @@ object EmailExportType extends Enumeration { val Digest = Value("digest") -} -object EmailExportService { - - private object VulnerabilityDescription{ - def apply(name: String, v: Option[Vulnerability]): VulnerabilityDescription = v.fold(UnknownVulnerabilityDescription(name))(new StandardVulnerabilityDescription(_)) - } - - private abstract class VulnerabilityDescription { - def name: String - def description: String - def cvssScore: Option[Double] - } - - private final class StandardVulnerabilityDescription(vulnerability: Vulnerability) extends VulnerabilityDescription { - override def name: String = vulnerability.name - override def description: String = vulnerability.description - override def cvssScore: Option[Double] = vulnerability.cvssScore - } - - private final class UnknownVulnerabilityDescription(override val name: String, link: String) extends VulnerabilityDescription { - override def description: String = s"Unknown vulnerability. Try looking at the following address for more details: $link" - override def cvssScore: Option[Double] = None - } - - private final class TotallyUnknownVulnerabilityDescription(override val name: String) extends VulnerabilityDescription { - override def description: String = s"Unknown vulnerability. Not even sure where to look for other details. Maybe Googling the identifier will help." - override def cvssScore: Option[Double] = None - } - - private object UnknownVulnerabilityDescription { - def apply(name: String): VulnerabilityDescription = name match { - case cveId if name startsWith "CVE-" => new UnknownVulnerabilityDescription(name, s"https://nvd.nist.gov/vuln/detail/$cveId") - case ossIndexId if name startsWith "OSSINDEX-" => new UnknownVulnerabilityDescription(name, s"https://ossindex.net/resource/vulnerability/$ossIndexId") - case other => new TotallyUnknownVulnerabilityDescription(other) - } - } - } class EmailExportService(from: String, nobodyInterestedContact: String, val exportType: EmailExportType.Value, odcService: OdcDbService, mailerClient: MailerClient, notificationService: VulnerabilityNotificationService, emailSendingExecutionContext: ExecutionContext, absolutizer: Absolutizer)(implicit executionContext: ExecutionContext) { // Maybe it is not the best place for exportType, but I am not sure if we want this to be configurable. If no, then we can get rid of it. If yes, we should refactor it. - import EmailExportService.VulnerabilityDescription - private def getEmail(loginInfo: LoginInfo) = loginInfo.providerKey // TODO: get the email in a cleaner way def recipientsForProjects(projects: Set[ReportInfo]) = for{ @@ -74,7 +35,7 @@ class EmailExportService(from: String, nobodyInterestedContact: String, val expo } } - def mailForVulnerabilityProjectsChange(vuln: Vulnerability, emailMessageId: EmailMessageId, diff: SetDiff[String], projects: ProjectsWithReports) = { + def mailForVulnerabilityProjectsChange(vuln: VulnerabilityOverview, emailMessageId: EmailMessageId, diff: SetDiff[String], projects: ProjectsWithReports) = { def showProjects(s: Set[String]) = s.map(p => "* " + (try{ friendlyProjectNameString(projects.parseUnfriendlyName(p)) @@ -119,7 +80,7 @@ class EmailExportService(from: String, nobodyInterestedContact: String, val expo def emailDigest(subscriber: LoginInfo, changes: Seq[Change], projects: ProjectsWithReports): Future[Email] = { val vulnNames = changes.map(_.vulnerabilityName).toSet for { - vulns <- Future.traverse(vulnNames.toSeq)(name => odcService.getVulnerabilityDetails(name).map(v => name -> VulnerabilityDescription(name, v))).map(_.toMap) + vulns <- Future.traverse(vulnNames.toSeq)(name => odcService.getVulnerabilityDetails(name).map(v => name -> VulnerabilityOverview(name, v))).map(_.toMap) groups = changes.groupBy(_.direction).withDefaultValue(Seq()) } yield { val changesMarks = Map(Direction.Added -> "❢", Direction.Removed -> "☑") @@ -131,9 +92,9 @@ class EmailExportService(from: String, nobodyInterestedContact: String, val expo text = "more info: "+link, html = Html("more info") ) - def vulnerabilityText(change: Change, vulnerability: VulnerabilityDescription): HtmlWithText = ( + def vulnerabilityText(change: Change, vulnerability: VulnerabilityOverview): HtmlWithText = ( heading(4)(s"${changesMarks(change.direction)} ${vulnerability.name}${vulnerability.cvssScore.fold("")(sev => s" (CVSS severity: $sev)")}") - + justHtml("

") + plainText(vulnerability.description) + justHtml("
") + justText("\n") + + justHtml("

") + plainText(vulnerability.descriptionAttempt) + justHtml("
") + justText("\n") + moreInfo(absolutizer.absolutize(routes.Statistics.vulnerability(vulnerability.name, None))) + justHtml("

") ) def vulnChanges(changes: Seq[Change]): HtmlWithText = diff --git a/app/services/IssueTrackerService.scala b/app/services/IssueTrackerService.scala index 5f5ff5e..cf2e2b6 100644 --- a/app/services/IssueTrackerService.scala +++ b/app/services/IssueTrackerService.scala @@ -2,7 +2,7 @@ package services import com.ysoft.odc.SetDiff import controllers.{ReportInfo, Vulnerability} -import models.ExportedVulnerability +import models.{ExportedVulnerability, VulnerabilityOverview} import scala.concurrent.Future @@ -10,6 +10,6 @@ trait IssueTrackerService { def reportVulnerability(vulnerability: Vulnerability, projects: Set[ReportInfo]): Future[ExportedVulnerability[String]] def ticketLink(ticket: String): String def ticketLink(ticket: ExportedVulnerability[String]): String = ticketLink(ticket.ticket) - def updateVulnerability(vuln: Vulnerability, diff: SetDiff[ReportInfo], ticket: String): Future[Unit] + def updateVulnerability(vuln: VulnerabilityOverview, diff: SetDiff[ReportInfo], ticket: String): Future[Unit] //def migrateOldIssues(): Future[Unit] } diff --git a/app/services/JiraIssueTrackerService.scala b/app/services/JiraIssueTrackerService.scala index bc308c4..c921e5d 100644 --- a/app/services/JiraIssueTrackerService.scala +++ b/app/services/JiraIssueTrackerService.scala @@ -5,7 +5,7 @@ import javax.inject.Inject import com.google.inject.name.Named import com.ysoft.odc.{Absolutizer, AtlassianAuthentication, SetDiff} import controllers.{ReportInfo, Vulnerability, friendlyProjectNameString, routes} -import models.ExportedVulnerability +import models.{ExportedVulnerability, StandardVulnerabilityOverview, VulnerabilityOverview} import play.api.Logger import play.api.libs.json.Json.JsValueWrapper import play.api.libs.json._ @@ -52,7 +52,7 @@ class JiraIssueTrackerService @Inject()(absolutizer: Absolutizer, @Named("jira-s private implicit val TransitionsFormats = Json.format[Transitions] override def reportVulnerability(vulnerability: Vulnerability, projects: Set[ReportInfo]): Future[ExportedVulnerability[String]] = throttler.throttle(api("issue").post(Json.obj( - "fields" -> (extractInitialFields(vulnerability) ++ extractManagedFields(vulnerability, projects)) + "fields" -> (extractInitialFields(vulnerability) ++ extractManagedFields(new StandardVulnerabilityOverview(vulnerability), projects, requiresDescription = true)) ))).map(response => // returns responses like {"id":"1234","key":"PROJ-6","self":"https://…/rest/api/2/issue/1234"} try{ val issueInfo = Json.reads[JiraNewIssueResponse].reads(response.json).get @@ -81,7 +81,7 @@ class JiraIssueTrackerService @Inject()(absolutizer: Absolutizer, @Named("jira-s } } - override def updateVulnerability(vuln: Vulnerability, diff: SetDiff[ReportInfo], ticket: String): Future[Unit] = { + override def updateVulnerability(vuln: VulnerabilityOverview, diff: SetDiff[ReportInfo], ticket: String): Future[Unit] = { val requiredTransitionOption = diff.whichNonEmpty match { case SetDiff.Selection.Old => noRelevantProjectAffectedTransitionNameOption case SetDiff.Selection.New | SetDiff.Selection.Both => newProjectAddedTransitionNameOption @@ -96,7 +96,7 @@ class JiraIssueTrackerService @Inject()(absolutizer: Absolutizer, @Named("jira-s } }.getOrElse(Future.successful(None)) val fieldsUpdateResult = throttler.throttle(api(s"issue/$ticket").put(obj( - "fields" -> extractManagedFields(vuln, diff.newSet) + "fields" -> extractManagedFields(vuln, diff.newSet, requiresDescription = false) ))).requireStatus(204).map{ resp => () } fieldsUpdateResult.flatMap { (_: Unit) => transitionOptionFuture flatMap { @@ -113,13 +113,13 @@ class JiraIssueTrackerService @Inject()(absolutizer: Absolutizer, @Named("jira-s "summary" -> s"${vulnerability.name} – ${vulnerability.cweOption.map(_ + ": ").getOrElse("")}${vulnerability.description.take(50).takeWhile(c => c != '\n' && c != '\r')}…" ) - private def extractManagedFields(vulnerability: Vulnerability, projects: Set[ReportInfo]): JsObject = { + private def extractManagedFields(vulnerability: VulnerabilityOverview, projects: Set[ReportInfo], requiresDescription: Boolean): JsObject = { val base = Json.obj( "issuetype" -> Json.obj( "id" -> vulnerabilityIssueType.toString - ), - "description" -> extractDescription(vulnerability) + ) ) + val descriptionObj = if(requiresDescription || vulnerability.isSureAboutDescription) Json.obj("description" -> extractDescription(vulnerability)) else Json.obj() val additionalFields = Seq[Option[(String, JsValueWrapper)]]( fields.cweId.map(id => id -> vulnerability.cweOption.fold("")(_.brief)), fields.linkId.map(id => id -> link(vulnerability)), @@ -129,7 +129,7 @@ class JiraIssueTrackerService @Inject()(absolutizer: Absolutizer, @Named("jira-s ) val additionalObj = Json.obj(additionalFields.flatten : _*) val constantObj = fields.constantFields.getOrElse(Json.obj()) - base ++ additionalObj ++ constantObj + base ++ descriptionObj ++ additionalObj ++ constantObj } @@ -137,9 +137,9 @@ class JiraIssueTrackerService @Inject()(absolutizer: Absolutizer, @Named("jira-s }*/ - private def extractDescription(vulnerability: Vulnerability): String = vulnerability.description + "\n\n" + s"Details: ${link(vulnerability)}" + private def extractDescription(vulnerability: VulnerabilityOverview): String = vulnerability.descriptionAttempt + "\n\n" + s"Details: ${link(vulnerability)}" - private def link(vulnerability: Vulnerability): String = { + private def link(vulnerability: VulnerabilityOverview): String = { absolutizer.absolutize(routes.Statistics.vulnerability(vulnerability.name, None)) } diff --git a/app/services/OdcDbService.scala b/app/services/OdcDbService.scala index 1ca7e6b..7f3008d 100644 --- a/app/services/OdcDbService.scala +++ b/app/services/OdcDbService.scala @@ -10,6 +10,7 @@ import _root_.org.owasp.dependencycheck.utils.{DependencyVersion, DependencyVers import com.github.nscala_time.time.Imports._ import com.google.inject.Inject import com.mockrunner.mock.jdbc.MockConnection +import models.VulnerabilityOverview import models.odc.tables._ import models.odc.{OdcProperty, Vulnerabilities} import play.api.Logger @@ -36,6 +37,8 @@ class OdcDbService @Inject()(@NamedDatabase("odc") protected val dbConfigProvide def getVulnerabilityDetails(name: String): Future[Option[com.ysoft.odc.Vulnerability]] = getVulnerabilityDetails(_.cve === name) + def getVulnerabilityDescription(name: String): Future[VulnerabilityOverview] = getVulnerabilityDetails(name).map(VulnerabilityOverview(name, _)) + private def getVulnerabilityDetails(cond: Vulnerabilities => Rep[Boolean]): Future[Option[com.ysoft.odc.Vulnerability]] = { db.run(vulnerabilities.filter(cond).result).map(_.headOption) flatMap { bareVulnOption => bareVulnOption.fold[Future[Option[com.ysoft.odc.Vulnerability]]](Future.successful(None)) { case (id, bareVuln) =>