Correctness fixes from full-repo review: query semantics, element projection, deletion, status codes

README.md

@@ -8,7 +8,7 @@ JDK 21 for development or running without docker
 ## Run
 
 ```
-cp src/main/resources/application.conf.example to src/main/resources/application.conf
+cp src/main/resources/application.conf.example src/main/resources/application.conf
 
 ./gradlew run
 ```

build.gradle.kts

@@ -11,7 +11,7 @@ repositories {
 }
 
 jacoco {
-    toolVersion = "0.8.12"
+    toolVersion = "0.8.15"
 }
 
 plugins {
@@ -34,7 +34,7 @@ sonar {
 dependencies {
     implementation(kotlin("stdlib"))
 
-    val ktorVersion = "3.4.3"
+    val ktorVersion = "3.5.1"
     implementation("io.ktor:ktor-client-core:$ktorVersion")
     implementation("io.ktor:ktor-client-content-negotiation:$ktorVersion")
     implementation("io.ktor:ktor-client-cio:$ktorVersion")
@@ -58,7 +58,7 @@ dependencies {
     implementation("io.ktor:ktor-server-netty:$ktorVersion")
     implementation("io.ktor:ktor-server-resources:$ktorVersion")
     implementation("io.ktor:ktor-server-status-pages:$ktorVersion")
-    val kotestVersion = "6.1.11"
+    val kotestVersion = "6.2.2"
     testImplementation("io.kotest:kotest-runner-junit5:$kotestVersion")
     testImplementation("io.kotest:kotest-assertions-core:$kotestVersion")
     testImplementation("io.kotest:kotest-assertions-json-jvm:$kotestVersion")
@@ -67,17 +67,17 @@ dependencies {
 
     testImplementation("io.ktor:ktor-server-test-host:$ktorVersion")
 
-    implementation("ch.qos.logback:logback-classic:1.5.18")
+    implementation("ch.qos.logback:logback-classic:1.5.38")
 
     val junitVersion = "5.13.1"
     testImplementation("org.junit.jupiter:junit-jupiter-engine:$junitVersion")
-    testImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-test:1.10.2")
+    testImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-test:1.11.0")
 
     // sign JWTs locally in tests with the same shared secret as the
     // layer1-service container (see src/test/resources/test.env)
-    testImplementation("com.auth0:java-jwt:4.5.0")
+    testImplementation("com.auth0:java-jwt:4.6.0")
 
-    val jenaVersion = "6.0.0"
+    val jenaVersion = "6.1.0"
     implementation("org.apache.jena:jena-arq:${jenaVersion}")
     implementation("org.apache.jena:jena-querybuilder:${jenaVersion}")
     // CommonSpec.beforeEach uses RDFConnection to reload cluster.trig via GSP

src/main/kotlin/org/openmbee/flexo/sysmlv2/AppMain.kt

@@ -60,7 +60,10 @@ fun Application.module() {
         //register(ContentType.Application.Json, GsonConverter())
     }
     install(CORS) {
-        allowCredentials = true
+        // credentials (the Authorization header) may only be shared with
+        // configured origins — anyHost()+allowCredentials would let any
+        // website replay a logged-in user's session
+        allowCredentials = GlobalFlexoConfig.corsAllowedOrigins.isNotEmpty()
 
         allowHeader(HttpHeaders.Authorization)
         allowHeader(HttpHeaders.ContentType)
@@ -86,7 +89,16 @@ fun Application.module() {
         exposeHeader(HttpHeaders.Link)
         exposeHeader("Flexo-Mms-Layer-1")
 
-        anyHost() // @TODO: make configuration
+        if (GlobalFlexoConfig.corsAllowedOrigins.isEmpty()) {
+            anyHost()
+        } else {
+            GlobalFlexoConfig.corsAllowedOrigins.forEach { origin ->
+                val scheme = origin.substringBefore("://", "")
+                val host = origin.substringAfter("://")
+                if (scheme.isEmpty()) allowHost(host, listOf("http", "https"))
+                else allowHost(host, listOf(scheme))
+            }
+        }
     }
     install(AutoHeadResponse) // see https://ktor.io/docs/autoheadresponse.html
     install(Compression, ApplicationCompressionConfiguration()) // see https://ktor.io/docs/compression.html
@@ -139,7 +151,10 @@ data class FlexoConfig(
     val org: String,
     val defaultTimeout: Long,
     val auth: String,
-    val basePath: String
+    val basePath: String,
+    // origins allowed for CORS ("scheme://host[:port]", space-separated);
+    // empty means any host, which browsers only honor without credentials
+    val corsAllowedOrigins: List<String> = emptyList()
 )
 
 /**
@@ -154,5 +169,7 @@ val Application.flexoConfig: FlexoConfig
         val defaultTimeout = property("flexo.defaultTimeout")?.getString()?.toLong() ?: 60L
         val auth = property("flexo.auth")?.getString() ?: ""
         val basePath = property("flexo.basePath")?.getString() ?: ""
-        return FlexoConfig(protocol, host, port, org, defaultTimeout, auth, basePath)
+        val corsAllowedOrigins = property("flexo.corsAllowedOrigins")?.getString()
+            ?.split(" ")?.filter { it.isNotBlank() } ?: emptyList()
+        return FlexoConfig(protocol, host, port, org, defaultTimeout, auth, basePath, corsAllowedOrigins)
     }

src/main/kotlin/org/openmbee/flexo/sysmlv2/Flexo.kt

@@ -35,6 +35,7 @@ fun escapeRdfDoubleQuotedLiteralContents(contents: String): String {
         .replace("\\", "\\\\")
         .replace("\"", "\\\"")
         .replace("\n", "\\n")
+        .replace("\r", "\\r")
 }
 
 private val RDF_IRI_ESCAPE_REGEX = "([\\x00-\\x20<>\"{}|^`\\\\]|%(?![0-9A-F][0-9A-F]))".toRegex()
@@ -335,10 +336,14 @@ class FlexoModelHandlerWithFocalNode(
 suspend fun RoutingContext.forward(flexoResponse: FlexoResponse) {
     val response = flexoResponse.response
 
-    call.respondText(response.bodyAsText(), response.contentType(), response.status) {
-        // forward the response headers
-        headersOf(*response.headers.names().map { name ->
-            name to headers.getAll(name).orEmpty()
-        }.toTypedArray())
+    // forward the upstream response headers (e.g. ETag, Location,
+    // WWW-Authenticate); content headers are managed by respondText and
+    // Date/Server belong to this server, not the upstream
+    response.headers.forEach { name, values ->
+        if (HttpHeaders.isUnsafe(name)
+            || name.equals(HttpHeaders.Date, ignoreCase = true)
+            || name.equals(HttpHeaders.Server, ignoreCase = true)) return@forEach
+        values.forEach { call.response.headers.append(name, it) }
     }
+    call.respondText(response.bodyAsText(), response.contentType(), response.status)
 }

src/main/kotlin/org/openmbee/flexo/sysmlv2/apis/BranchApi.kt

@@ -39,16 +39,15 @@ fun FlexoModelHandler.branchFromResponse(
 ): Branch? {
     val commitSuffix = outgoing[MMS.commit]?.resource()?.uri?.uriSuffix ?: return null
     val commit = Identified(commitSuffix)
+    // never fabricate created/name for malformed upstream data — invented
+    // values mask triplestore bugs (see issue #20)
     return Branch(
         atId = branchId,
         atType = Branch.AtType.Branch,
-        created = OffsetDateTime.parse(
-            outgoing[MMS.created]?.literal()
-                ?: OffsetDateTime.now().toString()
-        ),
+        created = OffsetDateTime.parse(outgoing[MMS.created]?.literal() ?: return null),
         owningProject = Identified(projectId),
         referencedCommit = commit,
-        name = outgoing [DCTerms.title]?.literal() ?: "",
+        name = outgoing[DCTerms.title]?.literal() ?: return null,
         head = commit
     )
 }
@@ -111,7 +110,10 @@ fun Route.BranchApi() {
         }
         // parse the response model, convert it to JSON, and reply to client
         val branch = flexoResponse.parseModel {
-            model.listSubjectsWithProperty(RDF.type, MMS.Branch).mapWith {
+            // soft-deleted branches are invisible
+            model.listSubjectsWithProperty(RDF.type, MMS.Branch).filterDrop {
+                it.hasProperty(SYSMLV2.DELETED)
+            }.mapWith {
                 branchFromResponse(it.outgoing(), path.projectId, it.uri.uriSuffix)
             }.toList().filterNotNull().firstOrNull()
         } ?: return@get call.respond(HttpStatusCode.NotFound)

src/main/kotlin/org/openmbee/flexo/sysmlv2/apis/CommitApi.kt

@@ -142,9 +142,18 @@ fun Route.CommitApi() {
             val projectResponse = flexoRequestGet {
                 orgPath("/repos/$projectId")
             }
+            // nonexistent/unauthorized project: forward instead of crashing
+            // on an empty model below
+            if (projectResponse.isFailure()) {
+                return@post forward(projectResponse)
+            }
             projectResponse.parseModel {
-                val outgoing = model.listSubjectsWithProperty(RDF.type, MMS.Repo).next()!!.outgoing()
-                branchId = outgoing[SYSMLV2.DEFAULT_BRANCH_ID]?.literal()?: "master"
+                val repos = model.listSubjectsWithProperty(RDF.type, MMS.Repo)
+                if (repos.hasNext()) {
+                    branchId = repos.next().outgoing()[SYSMLV2.DEFAULT_BRANCH_ID]?.literal()?: "master"
+                } else {
+                    branchId = "master"
+                }
             }
         }
         // each change (DataVersionRequest)
@@ -162,15 +171,20 @@ fun Route.CommitApi() {
             val identityId = identity?.atId
             var payloadId = payload?.getOrDefault("@id", null)?.jsonPrimitive?.content
             if (identityId != null && payloadId != null && identityId != payloadId) {
-                //bad, log error?
-                continue
+                // silently skipping the change would report a successful
+                // commit while discarding data
+                throw InvalidSysmlSerializationError(
+                    "identity @id '$identityId' does not match payload @id '$payloadId' at .change[$index]")
             }
             if (identityId == null && payloadId == null && payload != null) {
                 payloadId = generateId() // generate an id
             }
 
-            // subject node, target element
-            val elementNode: Node = SYSMLV2.element(identityId ?: payloadId!!).asNode()
+            // subject node, target element; the id is interpolated into
+            // SPARQL, so it must be validated
+            val elementId = identityId ?: payloadId!!
+            requireValidId(elementId, ".change[$index] element @id")
+            val elementNode: Node = SYSMLV2.element(elementId).asNode()
 
             // transform payload into property pairs
             mutableListOf<Pair<Property, Set<Node>>>().apply {
@@ -219,7 +233,10 @@ fun Route.CommitApi() {
                                         if(value[0] is JsonObject) {
                                             // create additional triples to link the elements
                                             add(SYSMLV2.prop(key) to value.jsonArray.map {
-                                                SYSMLV2.element(it.jsonObject["@id"]!!.jsonPrimitive.content).asNode()
+                                                val refId = it.jsonObject["@id"]?.jsonPrimitive?.content
+                                                    ?: throw InvalidSysmlSerializationError("Missing @id at .change[$index].$key")
+                                                requireValidId(refId, ".change[$index].$key.@id")
+                                                SYSMLV2.element(refId).asNode()
                                             }.toSet())
                                         }
                                         // and first element is a primitive
@@ -242,9 +259,12 @@ fun Route.CommitApi() {
                                     // @id reference
                                     if(valueObj.containsKey("@id")) {
                                         if(valueObj.keys.size > 1) {
-                                            throw Error("Unexpected extra keys at .${key}")
+                                            // client-input problem: must map to a 400, not a 500
+                                            throw InvalidSysmlSerializationError("Unexpected extra keys at .change[$index].$key")
                                         }
-                                        add(SYSMLV2.prop(key) to setOf(SYSMLV2.element(valueObj["@id"]!!.jsonPrimitive.content).asNode()))
+                                        val refId = valueObj["@id"]!!.jsonPrimitive.content
+                                        requireValidId(refId, ".change[$index].$key.@id")
+                                        add(SYSMLV2.prop(key) to setOf(SYSMLV2.element(refId).asNode()))
                                     }
                                     else {
                                         throw InvalidSysmlSerializationError("Unexpected JSON object at .change[$index].$key == ${Json.encodeToString(value)}")
@@ -264,6 +284,27 @@ fun Route.CommitApi() {
             }
         }
         if (replace == null || replace != "true") { //regular update
+            // deleted elements must not leave dangling references: remove
+            // incoming link triples, and the json: array annotations of the
+            // referencing property (they cannot be rewritten in SPARQL; the
+            // remaining link triples render as a best-effort array instead)
+            val deleteIncomingUpdate = if (deleteIncoming.isEmpty()) "" else """
+            delete {
+                ?ref_s ?ref_p ?deleted_n .
+                ?ref_s ?ref_ann ?ann_val .
+            } where {
+                values ?deleted_n {
+                    ${deleteIncoming.joinToString("\n").reindent(5)}
+                }
+                ?ref_s ?ref_p ?deleted_n .
+                optional {
+                    ?ref_s ?ref_ann ?ann_val .
+                    filter(strstarts(str(?ref_ann), "${SYSMLV2.ANNOTATION_JSON}"))
+                    filter(strafter(str(?ref_ann), "${SYSMLV2.ANNOTATION_JSON}") = strafter(str(?ref_p), "${SYSMLV2.VOCABULARY}"))
+                }
+            };
+            """
+
             // build SPARQL UPDATE string
             var sparqlUpdateString = """
             ${DEFAULT_PREFIX_MAPPING.nsPrefixMap.filter { (id, iri) ->
@@ -277,14 +318,15 @@ fun Route.CommitApi() {
             } where {
                 values ?element_n {
                     ${values.joinToString("\n").reindent(5)}
-                } 
+                }
                 optional {
                     ?element_n ?element_p ?element_o .
                 }
             };
+            $deleteIncomingUpdate
             insert data {
                 ${inserts.joinToString("\n\n").reindent(4)}
-            }           
+            }
             """
 
             // trim indent for better inspectability