From 56a84882a786850f8e16de9627186aa7f5bae8e6 Mon Sep 17 00:00:00 2001 From: Rick Parker Date: Mon, 21 Aug 2017 10:27:27 +0100 Subject: [PATCH] AMQP fixes: Don't attempt to include non-serializable interfaces in schemas (#1281) * Ignore interfaces that are not serializable * Annotate so test still works. * Make methods accessible to serializer. * Make sure interfaces are annotated in the carpenter. Expand tests to check whitelisting. Object is now whitelisted by default since it has no fields. * Prevented Object from being whitelisted but allow arrays of Objects (i.e. pretty much an untyped array) --- .../serialization/amqp/ObjectSerializer.kt | 2 +- .../serialization/amqp/PropertySerializer.kt | 1 + .../internal/serialization/amqp/Schema.kt | 2 +- .../serialization/amqp/SerializationHelper.kt | 15 +++++++++------ .../serialization/amqp/SerializerFactory.kt | 14 ++++++++------ .../serialization/amqp/SingletonSerializer.kt | 2 +- .../serialization/carpenter/ClassCarpenter.kt | 4 ++++ .../serialization/amqp/AMQPTestUtils.kt | 4 +++- .../amqp/DeserializeAndReturnEnvelopeTests.kt | 19 +++++++++++++++++++ .../amqp/DeserializeNeedingCarpentryTests.kt | 7 ++++++- 10 files changed, 53 insertions(+), 17 deletions(-) diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt index c942b2e6de..c9e610efaa 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt @@ -25,7 +25,7 @@ class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPSerial private val typeName = nameForType(clazz) override val typeDescriptor = "$DESCRIPTOR_DOMAIN:${fingerprintForType(type, factory)}" - private val interfaces = interfacesForSerialization(clazz) // TODO maybe this proves too much and we need annotations to restrict. + private val interfaces = interfacesForSerialization(clazz, factory) // We restrict to only those annotated or whitelisted internal val typeNotation: TypeNotation = CompositeType(typeName, null, generateProvides(), Descriptor(typeDescriptor, null), generateFields()) diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializer.kt index 143cd16c5b..d1cc5212f1 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/PropertySerializer.kt @@ -54,6 +54,7 @@ sealed class PropertySerializer(val name: String, val readMethod: Method, val re companion object { fun make(name: String, readMethod: Method, resolvedType: Type, factory: SerializerFactory): PropertySerializer { + readMethod.isAccessible = true if (SerializerFactory.isPrimitive(resolvedType)) { return when(resolvedType) { Char::class.java, Character::class.java -> AMQPCharPropertySerializer(name, readMethod) diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt index a67cb8e400..c7783b2376 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/Schema.kt @@ -403,6 +403,6 @@ private fun fingerprintForObject(type: Type, contextType: Type?, alreadySeen: Mu propertiesForSerialization(constructorForDeserialization(type), contextType ?: type, factory).fold(hasher.putUnencodedChars(name)) { orig, prop -> fingerprintForType(prop.resolvedType, type, alreadySeen, orig, factory).putUnencodedChars(prop.name).putUnencodedChars(if (prop.mandatory) NOT_NULLABLE_HASH else NULLABLE_HASH) } - interfacesForSerialization(type).map { fingerprintForType(it, type, alreadySeen, hasher, factory) } + interfacesForSerialization(type, factory).map { fingerprintForType(it, type, alreadySeen, hasher, factory) } return hasher } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationHelper.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationHelper.kt index a8c15461ca..f7e11e7dbd 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationHelper.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationHelper.kt @@ -103,23 +103,26 @@ private fun propertiesForSerializationFromAbstract(clazz: Class<*>, type: Type, return rc } -internal fun interfacesForSerialization(type: Type): List { +internal fun interfacesForSerialization(type: Type, serializerFactory: SerializerFactory): List { val interfaces = LinkedHashSet() - exploreType(type, interfaces) + exploreType(type, interfaces, serializerFactory) return interfaces.toList() } -private fun exploreType(type: Type?, interfaces: MutableSet) { +private fun exploreType(type: Type?, interfaces: MutableSet, serializerFactory: SerializerFactory) { val clazz = type?.asClass() if (clazz != null) { - if (clazz.isInterface) interfaces += type + if (clazz.isInterface) { + if(serializerFactory.isNotWhitelisted(clazz)) return // We stop exploring once we reach a branch that has no `CordaSerializable` annotation or whitelisting. + else interfaces += type + } for (newInterface in clazz.genericInterfaces) { if (newInterface !in interfaces) { - exploreType(resolveTypeVariables(newInterface, type), interfaces) + exploreType(resolveTypeVariables(newInterface, type), interfaces, serializerFactory) } } val superClass = clazz.genericSuperclass ?: return - exploreType(resolveTypeVariables(superClass, type), interfaces) + exploreType(resolveTypeVariables(superClass, type), interfaces, serializerFactory) } } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt index 64f9ba70c7..6ee9e5be37 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt @@ -4,10 +4,7 @@ import com.google.common.primitives.Primitives import com.google.common.reflect.TypeResolver import net.corda.core.serialization.ClassWhitelist import net.corda.core.serialization.CordaSerializable -import net.corda.nodeapi.internal.serialization.carpenter.CarpenterSchemas -import net.corda.nodeapi.internal.serialization.carpenter.ClassCarpenter -import net.corda.nodeapi.internal.serialization.carpenter.MetaCarpenter -import net.corda.nodeapi.internal.serialization.carpenter.carpenterSchema +import net.corda.nodeapi.internal.serialization.carpenter.* import org.apache.qpid.proton.amqp.* import java.io.NotSerializableException import java.lang.reflect.GenericArrayType @@ -224,7 +221,8 @@ class SerializerFactory(val whitelist: ClassWhitelist, cl : ClassLoader) { } else { findCustomSerializer(clazz, declaredType) ?: run { if (type.isArray()) { - whitelisted(type.componentType()) + // Allow Object[] since this can be quite common (i.e. an untyped array) + if(type.componentType() != Object::class.java) whitelisted(type.componentType()) if (clazz.componentType.isPrimitive) PrimArraySerializer.make(type, this) else ArraySerializer.make(type, this) } else if (clazz.kotlin.objectInstance != null) { @@ -258,11 +256,15 @@ class SerializerFactory(val whitelist: ClassWhitelist, cl : ClassLoader) { private fun whitelisted(type: Type) { val clazz = type.asClass()!! - if (!whitelist.hasListed(clazz) && !hasAnnotationInHierarchy(clazz)) { + if (isNotWhitelisted(clazz)) { throw NotSerializableException("Class $type is not on the whitelist or annotated with @CordaSerializable.") } } + // Ignore SimpleFieldAccess as we add it to anything we build in the carpenter. + internal fun isNotWhitelisted(clazz: Class<*>): Boolean = clazz == SimpleFieldAccess::class.java || + (!whitelist.hasListed(clazz) && !hasAnnotationInHierarchy(clazz)) + // Recursively check the class, interfaces and superclasses for our annotation. internal fun hasAnnotationInHierarchy(type: Class<*>): Boolean { return type.isAnnotationPresent(CordaSerializable::class.java) || diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SingletonSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SingletonSerializer.kt index 22b4a2da6e..dfc587a1dd 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SingletonSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SingletonSerializer.kt @@ -10,7 +10,7 @@ import java.lang.reflect.Type */ class SingletonSerializer(override val type: Class<*>, val singleton: Any, factory: SerializerFactory) : AMQPSerializer { override val typeDescriptor = "$DESCRIPTOR_DOMAIN:${fingerprintForType(type, factory)}" - private val interfaces = interfacesForSerialization(type) + private val interfaces = interfacesForSerialization(type, factory) private fun generateProvides(): List = interfaces.map { it.typeName } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenter.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenter.kt index a62be2da34..f62f5ca1b1 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenter.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenter.kt @@ -1,8 +1,10 @@ package net.corda.nodeapi.internal.serialization.carpenter +import net.corda.core.serialization.CordaSerializable import org.objectweb.asm.ClassWriter import org.objectweb.asm.MethodVisitor import org.objectweb.asm.Opcodes.* +import org.objectweb.asm.Type import java.lang.Character.isJavaIdentifierPart import java.lang.Character.isJavaIdentifierStart @@ -119,6 +121,7 @@ class ClassCarpenter(cl: ClassLoader = Thread.currentThread().contextClassLoader with(cw) { visit(V1_8, ACC_PUBLIC + ACC_ABSTRACT + ACC_INTERFACE, schema.jvmName, null, "java/lang/Object", interfaces) + visitAnnotation(Type.getDescriptor(CordaSerializable::class.java), true).visitEnd() generateAbstractGetters(schema) @@ -136,6 +139,7 @@ class ClassCarpenter(cl: ClassLoader = Thread.currentThread().contextClassLoader with(cw) { visit(V1_8, ACC_PUBLIC + ACC_SUPER, schema.jvmName, null, superName, interfaces.toTypedArray()) + visitAnnotation(Type.getDescriptor(CordaSerializable::class.java), true).visitEnd() generateFields(schema) generateConstructor(schema) diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPTestUtils.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPTestUtils.kt index e1f7db12c8..5cc7bbc125 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPTestUtils.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPTestUtils.kt @@ -2,12 +2,14 @@ package net.corda.nodeapi.internal.serialization.amqp import org.apache.qpid.proton.codec.Data import net.corda.nodeapi.internal.serialization.AllWhitelist +import net.corda.nodeapi.internal.serialization.EmptyWhitelist fun testDefaultFactory() = SerializerFactory(AllWhitelist, ClassLoader.getSystemClassLoader()) +fun testDefaultFactoryWithWhitelist() = SerializerFactory(EmptyWhitelist, ClassLoader.getSystemClassLoader()) class TestSerializationOutput( private val verbose: Boolean, - serializerFactory: SerializerFactory = SerializerFactory(AllWhitelist, ClassLoader.getSystemClassLoader())) + serializerFactory: SerializerFactory = testDefaultFactory()) : SerializationOutput(serializerFactory) { override fun writeSchema(schema: Schema, data: Data) { diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializeAndReturnEnvelopeTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializeAndReturnEnvelopeTests.kt index 6747b15db9..db59a9c2c8 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializeAndReturnEnvelopeTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializeAndReturnEnvelopeTests.kt @@ -1,5 +1,6 @@ package net.corda.nodeapi.internal.serialization.amqp +import net.corda.core.serialization.CordaSerializable import org.junit.Test import kotlin.test.assertEquals import kotlin.test.assertNotEquals @@ -43,4 +44,22 @@ class DeserializeAndReturnEnvelopeTests { assertNotEquals(null, obj.envelope.schema.types.find { it.name == classTestName("A") }) assertNotEquals(null, obj.envelope.schema.types.find { it.name == classTestName("B") }) } + + @Test + fun unannotatedInterfaceIsNotInSchema() { + @CordaSerializable + data class Foo(val bar: Int) : Comparable { + override fun compareTo(other: Foo): Int = bar.compareTo(other.bar) + } + + val a = Foo(123) + val factory = testDefaultFactoryWithWhitelist() + fun serialise(clazz: Any) = SerializationOutput(factory).serialize(clazz) + val obj = DeserializationInput(factory).deserializeAndReturnEnvelope(serialise(a)) + + assertTrue(obj.obj is Foo) + assertEquals(1, obj.envelope.schema.types.size) + assertNotEquals(null, obj.envelope.schema.types.find { it.name == classTestName("Foo") }) + assertEquals(null, obj.envelope.schema.types.find { it.name == "java.lang.Comparable<${classTestName("Foo")}>" }) + } } diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializeNeedingCarpentryTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializeNeedingCarpentryTests.kt index 585a362d15..3b8b2a5ec0 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializeNeedingCarpentryTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializeNeedingCarpentryTests.kt @@ -1,10 +1,12 @@ package net.corda.nodeapi.internal.serialization.amqp +import net.corda.core.serialization.CordaSerializable import org.junit.Test import kotlin.test.* import net.corda.nodeapi.internal.serialization.carpenter.* import net.corda.nodeapi.internal.serialization.AllWhitelist +@CordaSerializable interface I { fun getName() : String } @@ -26,7 +28,7 @@ class DeserializeNeedingCarpentryTests : AmqpCarpenterBase() { } val sf1 = testDefaultFactory() - val sf2 = testDefaultFactory() + val sf2 = testDefaultFactoryWithWhitelist() // Deserialize with whitelisting on to check that `CordaSerializable` annotation present. @Test fun verySimpleType() { @@ -115,6 +117,7 @@ class DeserializeNeedingCarpentryTests : AmqpCarpenterBase() { fun arrayOfTypes() { val clazz = ClassCarpenter().build(ClassSchema(testName(), mapOf("a" to NonNullableField(Int::class.java)))) + @CordaSerializable data class Outer (val a : Array) val outer = Outer (arrayOf ( @@ -185,6 +188,7 @@ class DeserializeNeedingCarpentryTests : AmqpCarpenterBase() { val nestedClass = cc.build (ClassSchema("nestedType", mapOf("name" to NonNullableField(String::class.java)))) + @CordaSerializable data class outer(val a: Any, val b: Any) val classInstance = outer ( @@ -204,6 +208,7 @@ class DeserializeNeedingCarpentryTests : AmqpCarpenterBase() { "v1" to NonNullableField(Int::class.java), "v2" to NonNullableField(Int::class.java)))) + @CordaSerializable data class outer (val l : List) val toSerialise = outer (listOf ( unknownClass.constructors.first().newInstance(1, 2),