WX-1385 Reject blob URLs with external SAS tokens as unparsable (#7347)

Author: aednichols

filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala

@@ -1,5 +1,6 @@
 package cromwell.filesystems.blob
 
+import akka.http.scaladsl.model.Uri
 import com.azure.storage.blob.nio.AzureBlobFileAttributes
 import com.google.common.net.UrlEscapers
 import cromwell.core.path.{NioPath, Path, PathBuilder}
@@ -21,6 +22,8 @@ object BlobPathBuilder {
     s"Malformed Blob URL for this builder: The endpoint $endpoint doesn't contain the expected host string '{SA}.blob.core.windows.net/'"
   def invalidBlobContainerMessage(endpoint: EndpointURL) =
     s"Malformed Blob URL for this builder: Could not parse container"
+  val externalToken =
+    "Rejecting pre-signed SAS URL so that filesystem selection falls through to HTTP filesystem"
   def parseURI(string: String): Try[URI] = Try(URI.create(UrlEscapers.urlFragmentEscaper().escape(string)))
   def parseStorageAccount(uri: URI): Try[StorageAccountName] = uri.getHost
     .split("\\.")
@@ -50,19 +53,34 @@ object BlobPathBuilder {
   def validateBlobPath(string: String): BlobPathValidation = {
     val blobValidation = for {
       testUri <- parseURI(string)
-      testEndpoint = EndpointURL(testUri.getScheme + "://" + testUri.getHost())
-      testStorageAccount <- parseStorageAccount(testUri)
+      testEndpoint = EndpointURL(testUri.getScheme + "://" + testUri.getHost)
+      _ <- parseStorageAccount(testUri)
       testContainer = testUri.getPath.split("/").find(_.nonEmpty)
-      isBlobHost = testUri.getHost().contains(blobHostnameSuffix) && testUri.getScheme().contains("https")
-      blobPathValidation = (isBlobHost, testContainer) match {
-        case (true, Some(container)) =>
+      isBlobHost = testUri.getHost.contains(blobHostnameSuffix) && testUri.getScheme.contains("https")
+      hasToken = hasSasToken(string)
+      blobPathValidation = (isBlobHost, testContainer, hasToken) match {
+        case (true, Some(container), false) =>
           ValidBlobPath(testUri.getPath.replaceFirst("/" + container, ""), BlobContainerName(container), testEndpoint)
-        case (false, _) => UnparsableBlobPath(new MalformedURLException(invalidBlobHostMessage(testEndpoint)))
-        case (true, None) => UnparsableBlobPath(new MalformedURLException(invalidBlobContainerMessage(testEndpoint)))
+        case (false, _, false) =>
+          UnparsableBlobPath(new MalformedURLException(invalidBlobHostMessage(testEndpoint)))
+        case (true, None, false) =>
+          UnparsableBlobPath(new MalformedURLException(invalidBlobContainerMessage(testEndpoint)))
+        case (_, _, true) =>
+          UnparsableBlobPath(new IllegalArgumentException(externalToken))
       }
     } yield blobPathValidation
     blobValidation recover { case t => UnparsableBlobPath(t) } get
   }
+
+  private def hasSasToken(uri: Uri) = {
+    // These keys are required for all SAS tokens.
+    // https://learn.microsoft.com/en-us/rest/api/storageservices/create-service-sas#construct-a-service-sas
+    val SignedVersionKey = "sv"
+    val SignatureKey = "sig"
+
+    val query = uri.query().toMap
+    query.isDefinedAt(SignedVersionKey) && query.isDefinedAt(SignatureKey)
+  }
 }
 
 class BlobPathBuilder()(private val fsm: BlobFileSystemManager) extends PathBuilder {

filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala

@@ -27,6 +27,23 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar {
     }
   }
 
+  it should "reject a path that is otherwise valid, but has a preexisting SAS token" in {
+    import cromwell.filesystems.blob.BlobPathBuilder.UnparsableBlobPath
+
+    // The `.asInstanceOf[UnparsableBlobPath].errorMessage.getMessage` malarkey is necessary
+    // because Java exceptions compare by reference, while strings are by value
+
+    val sasBlob = "https://lz304a1e79fd7359e5327eda.blob.core.windows.net/sc-705b830a-d699-478e-9da6-49661b326e77" +
+      "?sv=2021-12-02&spr=https&st=2023-12-13T20%3A27%3A55Z&se=2023-12-14T04%3A42%3A55Z&sr=c&sp=racwdlt&sig=blah&rscd=foo"
+    BlobPathBuilder.validateBlobPath(sasBlob).asInstanceOf[UnparsableBlobPath].errorMessage.getMessage should equal(
+      UnparsableBlobPath(
+        new IllegalArgumentException(
+          "Rejecting pre-signed SAS URL so that filesystem selection falls through to HTTP filesystem"
+        )
+      ).errorMessage.getMessage
+    )
+  }
+
   it should "provide a readable error when getting an illegal nioPath" in {
     val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount")
     val container = BlobContainerName("container")
@@ -39,8 +56,9 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar {
     testException should contain(exception)
   }
 
-  private def testBlobNioStringCleaning(input: String, expected: String) =
-    BlobPath.cleanedNioPathString(input) shouldBe expected
+  // The following tests use the `centaurtesting` account injected into CI. They depend on access to the
+  // container specified below. You may need to log in to az cli locally to get them to pass.
+  private val subscriptionId: SubscriptionId = SubscriptionId(UUID.fromString("62b22893-6bc1-46d9-8a90-806bb3cce3c9"))
 
   it should "clean the NIO path string when it has a garbled http protocol" in {
     testBlobNioStringCleaning(
@@ -69,10 +87,6 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar {
       ""
     )
   }
-
-  // The following tests use the `centaurtesting` account injected into CI. They depend on access to the
-  // container specified below. You may need to log in to az cli locally to get them to pass.
-  private val subscriptionId: SubscriptionId = SubscriptionId(UUID.fromString("62b22893-6bc1-46d9-8a90-806bb3cce3c9"))
   private val endpoint: EndpointURL = BlobPathBuilderSpec.buildEndpoint("centaurtesting")
   private val container: BlobContainerName = BlobContainerName("test-blob")
 
@@ -82,6 +96,9 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar {
     new BlobPathBuilder()(fsm)
   }
 
+  private def testBlobNioStringCleaning(input: String, expected: String) =
+    BlobPath.cleanedNioPathString(input) shouldBe expected
+
   it should "read md5 from small files <5g" in {
     val builder = makeBlobPathBuilder()
     val evalPath = "/testRead.txt"

project/Dependencies.scala

@@ -499,7 +499,7 @@ object Dependencies {
     List("scalatest", "mysql", "mariadb", "postgresql")
       .map(name => "com.dimafeng" %% s"testcontainers-scala-$name" % testContainersScalaV % Test)
 
-  val blobFileSystemDependencies: List[ModuleID] = azureDependencies ++ wsmDependencies
+  val blobFileSystemDependencies: List[ModuleID] = azureDependencies ++ wsmDependencies ++ akkaHttpDependencies
 
   val s3FileSystemDependencies: List[ModuleID] = junitDependencies