fix(core): enforce identifier validation on AST JSON (#591)

Signed-off-by: Matt Roberts <[email protected]>

packages/concerto-core/lib/introspect/classdeclaration.js

@@ -79,6 +79,12 @@ class ClassDeclaration extends Decorated {
     process() {
         super.process();
 
+        if (!ModelUtil.isValidIdentifier(this.ast.name)){
+            throw new IllegalModelException(`Invalid class name '${this.ast.name}'`, this.modelFile, this.ast.location);
+        }
+
+        const reservedProperties = ['$class', '$identifier', '$timestamp'];
+
         this.name = this.ast.name;
         this.properties = [];
         this.superType = null;
@@ -111,8 +117,8 @@ class ClassDeclaration extends Decorated {
         for (let n = 0; n < this.ast.properties.length; n++) {
             let thing = this.ast.properties[n];
 
-            if(thing.name && thing.name.startsWith('$')) {
-                throw new IllegalModelException(`Invalid field name ${thing.name}`, this.modelFile, this.ast.location);
+            if(reservedProperties.includes(thing.name)) {
+                throw new IllegalModelException(`Invalid field name '${thing.name}'`, this.modelFile, this.ast.location);
             }
 
             if (thing.$class === `${MetaModelNamespace}.RelationshipProperty`) {

packages/concerto-core/lib/introspect/modelfile.js

@@ -679,6 +679,14 @@ class ModelFile extends Decorated {
      */
     fromAst(ast) {
         const nsInfo = ModelUtil.parseNamespace(ast.namespace);
+
+        const namespaceParts = nsInfo.name.split('.');
+        namespaceParts.forEach(part => {
+            if (!ModelUtil.isValidIdentifier(part)){
+                throw new IllegalModelException(`Invalid namespace part '${part}'`, this.modelFile, this.ast.location);
+            }
+        });
+
         this.namespace = ast.namespace;
         this.version = nsInfo.version;
 

packages/concerto-core/lib/introspect/property.js

@@ -17,6 +17,7 @@
 const { MetaModelNamespace } = require('@accordproject/concerto-metamodel');
 
 const ModelUtil = require('../modelutil');
+const IllegalModelException = require('./illegalmodelexception');
 const Decorated = require('./decorated');
 
 // Types needed for TypeScript generation.
@@ -75,6 +76,10 @@ class Property extends Decorated {
     process() {
         super.process();
 
+        if (!ModelUtil.isValidIdentifier(this.ast.name)){
+            throw new IllegalModelException(`Invalid property name '${this.ast.name}'`, this.modelFile, this.ast.location);
+        }
+
         this.name = this.ast.name;
         this.decorator = null;
 

packages/concerto-core/lib/modelutil.js

@@ -18,6 +18,8 @@ const { MetaModelUtil } = require('@accordproject/concerto-metamodel');
 const semver = require('semver');
 const Globalize = require('./globalize');
 
+const ID_REGEX = /^(\p{Lu}|\p{Ll}|\p{Lt}|\p{Lm}|\p{Lo}|\p{Nl}|\$|_|\\u[0-9A-Fa-f]{4})(?:\p{Lu}|\p{Ll}|\p{Lt}|\p{Lm}|\p{Lo}|\p{Nl}|\$|_|\\u[0-9A-Fa-f]{4}|\p{Mn}|\p{Mc}|\p{Nd}|\p{Pc}|\u200C|\u200D)*$/u;
+
 /**
  * Internal Model Utility Class
  * <p><a href="./diagrams-private/modelutil.svg"><img src="./diagrams-private/modelutil.svg" style="height:100%;"/></a></p>
@@ -32,14 +34,12 @@ class ModelUtil {
      * @return {string} - the string after the last dot
      */
     static getShortName(fqn) {
-        //console.log('toShortName ' + name );
         let result = fqn;
         let dotIndex = fqn.lastIndexOf('.');
         if (dotIndex > -1) {
             result = fqn.substr(dotIndex + 1);
         }
 
-        //console.log('result ' + result );
         return result;
     }
 
@@ -184,6 +184,15 @@ class ModelUtil {
         return (declaration !== null && declaration.isScalarDeclaration?.());
     }
 
+    /**
+     * Return true if the name is a valid Concerto identifier
+     * @param {string} name - the name of the identifier to test.
+     * @returns {boolean} true if the identifier is valid.
+     */
+    static isValidIdentifier(name) {
+        return ID_REGEX.test(name);
+    }
+
     /**
      * Get the fully qualified name of a type.
      * @param {string} namespace - namespace of the type.

packages/concerto-core/test/introspect/classdeclaration.js

@@ -58,6 +58,15 @@ describe('ClassDeclaration', () => {
             }).should.throw(/Unrecognised model element/);
         });
 
+        it('should throw for a bad identifier', () => {
+            (() => {
+                new ClassDeclaration(modelFile, {
+                    name: '2nd',
+                    properties: []
+                });
+            }).should.throw(/Invalid class name '2nd'/);
+        });
+
     });
 
     describe('#validate', () => {

packages/concerto-core/test/introspect/identifieddeclaration.js

@@ -182,7 +182,22 @@ asset Order identified by sku {
                 asset Order {
                     o String $identifier
                 }`, 'test.cto');
-            }).should.throw(/Invalid field name \$identifier/);
+            }).should.throw(/Invalid field name '\$identifier'/);
+        });
+
+        it('should allow field called $foo', () => {
+            const mm = new ModelManager();
+
+            mm.addCTOModel( `
+                namespace test
+
+                asset Order {
+                    o String $foo
+                }`, 'test.cto');
+
+            const order = mm.getType('test.Order');
+            order.should.not.be.null;
+            order.getProperties().length.should.equal(2); // XXX Assets always have an identifier
         });
 
         it('should allow abstract assets without an identifier', () => {

packages/concerto-core/test/introspect/modelfile.js

@@ -136,6 +136,20 @@ describe('ModelFile', () => {
             (mf.getImportURI('org.doge.Foo') === null).should.be.true;
         });
 
+        it('should throw for a bad namespace part', () => {
+            const ast = {
+                $class: `${MetaModelNamespace}.Model`,
+                namespace: 'org.foo-bar',
+                imports: [],
+                declarations: [ ]
+            };
+            sandbox.stub(Parser, 'parse').returns(ast);
+
+            (() => {
+                ParserUtil.newModelFile(modelManager, 'fake definitions');
+            }).should.throw(/Invalid namespace part 'foo-bar'/);
+        });
+
         it('should throw for a wildcard import when strict is true', () => {
             const strictModelManager = new ModelManager({ strict: true });
 

packages/concerto-core/test/introspect/property.js

@@ -90,6 +90,15 @@ describe('Property', () => {
             p.array.should.equal(true);
         });
 
+        it('should throw for a bad property identifier', () => {
+            (() => {
+                new Property(mockClassDeclaration, {
+                    $class: `${MetaModelNamespace}.StringProperty`,
+                    name: '1st',
+                });
+            }).should.throw(/Invalid property name '1st'/);
+        });
+
     });
 
     describe('#hasInstance', () => {

packages/concerto-core/types/lib/modelutil.d.ts

@@ -98,6 +98,12 @@ declare class ModelUtil {
      * @private
      */
     private static isScalar;
+    /**
+     * Return true if the name is a valid Concerto identifier
+     * @param {string} name - the name of the identifier to test.
+     * @returns {boolean} true if the identifier is valid.
+     */
+    static isValidIdentifier(name: string): boolean;
     /**
      * Get the fully qualified name of a type.
      * @param {string} namespace - namespace of the type.