Merge pull request #3525 from magda-io/issue/3193

Issue/3193 auto-remove Null byte from JSON input so it won't be rejected by registry

CHANGES.md

@@ -1,5 +1,9 @@
 # CHANGELOG
 
+## v3.0.3
+
+- #3193 auto-remove Null byte from JSON input so it won't be rejected by registry
+
 ## v3.0.2
 
 - #3513 Fixed Indexer reindex trigger job doesn't start

magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/AspectsService.scala

@@ -23,8 +23,10 @@ import scalikejdbc._
 import spray.json.JsObject
 
 import scala.util.{Failure, Success}
-
-import au.csiro.data61.magda.directives.CommonDirectives.onCompleteBlockingTask
+import au.csiro.data61.magda.directives.CommonDirectives.{
+  onCompleteBlockingTask,
+  sanitizedJsonEntity
+}
 
 class AspectsService(
     config: Config,
@@ -101,7 +103,7 @@ class AspectsService(
     pathEnd {
       requireUserId { userId =>
         requiresSpecifiedTenantId { tenantId =>
-          entity(as[AspectDefinition]) { aspect =>
+          sanitizedJsonEntity(as[AspectDefinition]) { aspect =>
             requirePermission(
               authClient,
               "object/aspect/create",
@@ -223,7 +225,7 @@ class AspectsService(
       {
         requireUserId { userId =>
           requiresSpecifiedTenantId { tenantId =>
-            entity(as[AspectDefinition]) { aspect =>
+            sanitizedJsonEntity(as[AspectDefinition]) { aspect =>
               requireAspectUpdateOrCreateWhenNonExistPermission(
                 authClient,
                 id,
@@ -345,7 +347,7 @@ class AspectsService(
     path(Segment) { id: String =>
       requireUserId { userId =>
         requiresSpecifiedTenantId { tenantId =>
-          entity(as[JsonPatch]) { aspectPatch =>
+          sanitizedJsonEntity(as[JsonPatch]) { aspectPatch =>
             requireAspectUpdateOrCreateWhenNonExistPermission(
               authClient,
               id,

magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/RecordAspectsService.scala

@@ -6,14 +6,12 @@ import akka.http.scaladsl.model.headers.RawHeader
 import akka.http.scaladsl.server.Directives._
 import akka.http.scaladsl.server.Route
 import akka.stream.Materializer
-import au.csiro.data61.magda.directives.AuthDirectives.{requireUserId}
-import au.csiro.data61.magda.directives.TenantDirectives.{
-  requiresSpecifiedTenantId
-}
+import au.csiro.data61.magda.directives.AuthDirectives.requireUserId
+import au.csiro.data61.magda.directives.TenantDirectives.requiresSpecifiedTenantId
 import au.csiro.data61.magda.model.Registry._
 import au.csiro.data61.magda.registry.Directives.{
-  requireRecordAspectUpdatePermission,
-  requireDeleteRecordAspectPermission
+  requireDeleteRecordAspectPermission,
+  requireRecordAspectUpdatePermission
 }
 import com.typesafe.config.Config
 import gnieh.diffson.sprayJson._
@@ -24,8 +22,10 @@ import scalikejdbc.DB
 import spray.json.JsObject
 
 import scala.util.{Failure, Success}
-
-import au.csiro.data61.magda.directives.CommonDirectives.onCompleteBlockingTask
+import au.csiro.data61.magda.directives.CommonDirectives.{
+  onCompleteBlockingTask,
+  sanitizedJsonEntity
+}
 
 @Path("/records/{recordId}/aspects")
 @io.swagger.annotations.Api(
@@ -154,7 +154,7 @@ class RecordAspectsService(
       (recordId: String, aspectId: String) =>
         requireUserId { userId =>
           requiresSpecifiedTenantId { tenantId =>
-            entity(as[JsObject]) { aspect =>
+            sanitizedJsonEntity(as[JsObject]) { aspect =>
               parameters(
                 'merge.as[Boolean].?
               ) { merge =>
@@ -392,7 +392,7 @@ class RecordAspectsService(
       (recordId: String, aspectId: String) =>
         requireUserId { userId =>
           requiresSpecifiedTenantId { tenantId =>
-            entity(as[JsonPatch]) { aspectPatch =>
+            sanitizedJsonEntity(as[JsonPatch]) { aspectPatch =>
               requireRecordAspectUpdatePermission(
                 authClient,
                 recordId,

magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/RecordsService.scala

@@ -35,10 +35,10 @@ import spray.json.JsObject
 import scala.concurrent.{Await, ExecutionContext, Future}
 import scala.concurrent.duration._
 import scala.util.{Failure, Success}
-
 import au.csiro.data61.magda.directives.CommonDirectives.{
   onCompleteBlockingTask,
-  onCompleteBlockingTaskIn
+  onCompleteBlockingTaskIn,
+  sanitizedJsonEntity
 }
 
 @Path("/records")
@@ -418,7 +418,7 @@ class RecordsService(
     path(Segment) { id: String =>
       requireUserId { userId =>
         requiresSpecifiedTenantId { tenantId =>
-          entity(as[Record]) { recordIn =>
+          sanitizedJsonEntity(as[Record]) { recordIn =>
             parameters(
               'merge.as[Boolean].?
             ) { merge =>
@@ -577,7 +577,7 @@ class RecordsService(
     path(Segment) { id: String =>
       requireUserId { userId =>
         requiresSpecifiedTenantId { tenantId =>
-          entity(as[JsonPatch]) { recordPatch =>
+          sanitizedJsonEntity(as[JsonPatch]) { recordPatch =>
             requireRecordUpdateOrCreateWhenNonExistPermission(
               authClient,
               id,
@@ -707,7 +707,7 @@ class RecordsService(
   def patchRecords: Route = patch {
     requireUserId { userId =>
       requiresSpecifiedTenantId { tenantId =>
-        entity(as[PatchRecordsRequest]) { requestData =>
+        sanitizedJsonEntity(as[PatchRecordsRequest]) { requestData =>
           withAuthDecision(
             authClient,
             AuthDecisionReqConfig("object/record/update")
@@ -831,7 +831,7 @@ class RecordsService(
     path("aspects" / Segment) { (aspectId: String) =>
       requireUserId { userId =>
         requiresSpecifiedTenantId { tenantId =>
-          entity(as[PutRecordsAspectRequest]) { requestData =>
+          sanitizedJsonEntity(as[PutRecordsAspectRequest]) { requestData =>
             parameters(
               'merge.as[Boolean].?
             ) { merge =>
@@ -1080,7 +1080,7 @@ class RecordsService(
     requireUserId { userId =>
       requiresSpecifiedTenantId { tenantId =>
         pathEnd {
-          entity(as[Record]) { record =>
+          sanitizedJsonEntity(as[Record]) { record =>
             requirePermission(
               authClient,
               "object/record/create",

magda-registry-api/src/test/scala/au/csiro/data61/magda/registry/RecordsServiceSpec.scala

@@ -2,7 +2,8 @@ package au.csiro.data61.magda.registry
 
 import java.net.URLEncoder
 import akka.event.LoggingAdapter
-import akka.http.scaladsl.model.StatusCodes
+import akka.http.javadsl.model.RequestEntity
+import akka.http.scaladsl.model.{ContentTypes, HttpEntity, StatusCodes}
 import au.csiro.data61.magda.model.Registry._
 import au.csiro.data61.magda.model.TenantId._
 import gnieh.diffson._
@@ -3913,6 +3914,116 @@ class RecordsServiceSpec extends ApiSpec {
         }
       }
 
+      it("can add a new record with Null Byte with no error") { param =>
+        val aspectDefinition = AspectDefinition("test", "test", None)
+        Post("/v0/aspects", aspectDefinition) ~> addUserId() ~> addTenantIdHeader(
+          TENANT_1
+        ) ~> param.api(role).routes ~> check {
+          status shouldEqual StatusCodes.OK
+        }
+        var eventId: Option[String] = None
+        val record =
+          Record(
+            "testId",
+            "testName",
+            Map(
+              "test" -> JsObject(
+                // null bytes should be removed from field values
+                "a" -> JsString("abc\u0000edf"),
+                // null bytes should be removed from field name as well
+                "b\u0000c" -> JsString("bc\u0000123")
+              )
+            ),
+            Some("tag")
+          )
+        val expectedRecord = record.copy(
+          tenantId = Some(TENANT_1),
+          aspects = Map(
+            "test" -> JsObject(
+              "a" -> JsString("abcedf"),
+              "bc" -> JsString("bc123")
+            )
+          )
+        )
+        val reqBody = record.toJson.toString()
+        val reqEntity = HttpEntity(ContentTypes.`application/json`, reqBody)
+        Post("/v0/records", reqEntity) ~> addUserId() ~> addTenantIdHeader(
+          TENANT_1
+        ) ~> param.api(role).routes ~> check {
+          status shouldEqual StatusCodes.OK
+          val r = responseAs[Record]
+          r shouldEqual expectedRecord
+          r.aspects
+            .get("test")
+            .flatMap(x => x.fields.get("a").map(_.convertTo[String]))
+            .get shouldBe "abcedf"
+          r.aspects
+            .get("test")
+            .flatMap(x => x.fields.get("bc").map(_.convertTo[String]))
+            .get shouldBe "bc123"
+          // --- try to retrieve eventId generated because of this request
+          eventId = header("x-magda-event-id").map(_.value())
+        }
+
+        Get("/v0/records/testId?aspect=test") ~> addTenantIdHeader(TENANT_1) ~> param
+          .api(role)
+          .routes ~> check {
+          status shouldEqual StatusCodes.OK
+          val r = responseAs[Record]
+          r shouldEqual expectedRecord
+          r.aspects
+            .get("test")
+            .flatMap(x => x.fields.get("a").map(_.convertTo[String]))
+            .get shouldBe "abcedf"
+          r.aspects
+            .get("test")
+            .flatMap(x => x.fields.get("bc").map(_.convertTo[String]))
+            .get shouldBe "bc123"
+        }
+
+        Get(s"/v0/records/${record.id}/history") ~> addTenantIdHeader(TENANT_1) ~> param
+          .api(role)
+          .routes ~> check {
+          status shouldEqual StatusCodes.OK
+
+          val eventsPage = responseAs[EventsPage]
+          eventsPage.events.length shouldEqual 2
+          eventsPage.events(0).userId shouldEqual Some(USER_ID)
+          eventsPage.events(0).eventType shouldEqual EventType.CreateRecord
+          eventsPage
+            .events(0)
+            .data
+            .fields("recordId")
+            .convertTo[String] shouldEqual record.id
+          eventsPage.events(1).userId shouldEqual Some(USER_ID)
+          eventsPage
+            .events(1)
+            .eventType shouldEqual EventType.CreateRecordAspect
+          eventsPage
+            .events(1)
+            .data
+            .fields("aspect")
+            .asJsObject
+            .fields("a")
+            .convertTo[String] shouldEqual "abcedf"
+          eventsPage
+            .events(1)
+            .data
+            .fields("aspect")
+            .asJsObject
+            .fields("bc")
+            .convertTo[String] shouldEqual "bc123"
+          // --- check if the event id match the event id in previous POST response header
+          eventId.get shouldEqual eventsPage.events(1).id.get.toString
+        }
+
+        Get("/v0/records/testId") ~> addTenantIdHeader(TENANT_2) ~> param
+          .api(role)
+          .routes ~> check {
+          status shouldEqual StatusCodes.NotFound
+        }
+      }
+
       it(
         "can add two new records with the same record IDs by different tenants"
       ) { implicit param =>

magda-scala-common/src/main/scala/au/csiro/data61/magda/directives/CommonDirectives.scala

@@ -2,7 +2,22 @@ package au.csiro.data61.magda.directives
 
 import akka.http.scaladsl.server.Directives._
 import akka.http.scaladsl.server.{Directive0, _}
+
 import scala.concurrent.Future
+import akka.http.scaladsl.model.{
+  ContentTypes,
+  ExceptionWithErrorInfo,
+  HttpEntity
+}
+import akka.http.scaladsl.unmarshalling.Unmarshaller.UnsupportedContentTypeException
+import akka.http.scaladsl.unmarshalling.{
+  FromRequestUnmarshaller,
+  Unmarshaller,
+  Unmarshal
+}
+import scala.util.{Failure, Success}
+import akka.http.scaladsl.server.directives.RouteDirectives.reject
+import au.csiro.data61.magda.util.StringUtils.ExtraStringHelperFunctions
 
 object CommonDirectives {
 
@@ -21,4 +36,57 @@ object CommonDirectives {
       }
     }
 
+  /**
+    * Sanitize Json input to remove any Null bytes in Json string as postgreSQL won't accept it
+    *
+    * Example: sanitizedJsonEntity(as[Record])
+    *
+    * @param um Unmarshaller
+    * @tparam T
+    * @return
+    */
+  def sanitizedJsonEntity[T](um: FromRequestUnmarshaller[T]): Directive1[T] =
+    extractRequestContext.flatMap[Tuple1[T]] { ctx =>
+      import ctx.executionContext
+      import ctx.materializer
+
+      val unmarshalled: Future[T] = Unmarshal(ctx.request).to[String].flatMap {
+        jsonString =>
+          val ent = HttpEntity(
+            ContentTypes.`application/json`,
+            jsonString.removeNullByteFromJsonString
+          )
+          um(ctx.request.mapEntity(_ => ent))
+      }
+
+      onComplete(unmarshalled).flatMap {
+        case Success(value) =>
+          provide(value)
+        case Failure(RejectionError(r)) =>
+          reject(r)
+        case Failure(Unmarshaller.NoContentException) =>
+          reject(RequestEntityExpectedRejection)
+        case Failure(x: UnsupportedContentTypeException) =>
+          reject(
+            UnsupportedRequestContentTypeRejection(
+              x.supported,
+              x.actualContentType
+            )
+          )
+        case Failure(x: IllegalArgumentException) =>
+          reject(ValidationRejection(x.getMessage.nullAsEmpty, Some(x)))
+        case Failure(x: ExceptionWithErrorInfo) =>
+          reject(
+            MalformedRequestContentRejection(
+              x.info.format(ctx.settings.verboseErrorMessages),
+              x
+            )
+          )
+        case Failure(x) =>
+          reject(MalformedRequestContentRejection(x.getMessage.nullAsEmpty, x))
+      }
+    } & cancelRejections(
+      RequestEntityExpectedRejection.getClass,
+      classOf[UnsupportedRequestContentTypeRejection]
+    )
 }

magda-scala-common/src/main/scala/au/csiro/data61/magda/util/StringUtils.scala

@@ -12,5 +12,16 @@ object StringUtils {
     def toUrlSegment = urlSegmentEncoder.encode(s, "utf-8")
 
     def toQueryStringVal = urlQsValEncoder.encode(s, "utf-8")
+
+    def removeNullByte: String = {
+      s.replace("\u0000", "")
+    }
+
+    def removeNullByteFromJsonString: String = {
+      s.replace("\u0000", "").replace("\\u0000", "")
+    }
+
+    def nullAsEmpty: String =
+      if (s eq null) "" else s
   }
 }