added: detect disallowed self-referencing in form-logic, closes #44

CHANGELOG.md

@@ -2,8 +2,11 @@
 All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).
 
-[Unreleased]
+[1.5.0] - 2018-12-21
 ---------------------
+#### Added
+- Detect disallowed logic references to node itself.
+
 #### Changed
 - Added version property to CommonJS module validation output.
 - Updated to Enketo Core 5.0.x

package.json

@@ -1,6 +1,6 @@
 {
     "name": "enketo-validate",
-    "version": "1.4.0",
+    "version": "1.5.0",
     "description": "An XForm validator around Enketo's form engine",
     "main": "src/validator.js",
     "bin": "./validate",
@@ -40,4 +40,4 @@
         "mocha": "^5.0.1",
         "pkg": "^4.3.0"
     }
-}
+}

src/validator.js

@@ -58,14 +58,26 @@ let validate = ( xformStr, options = {} ) => {
             [ 'calculate', 'constraint', 'relevant', 'required' ].forEach( logicName => {
                 const logicExpr = bind.getAttribute( logicName );
                 const calculation = logicName === 'calculate';
+
                 if ( logicExpr ) {
+                    const friendlyLogicName = calculation ? 'Calculation' : logicName[ 0 ].toUpperCase() + logicName.substring( 1 );;
+
                     try {
                         xform.enketoEvaluate( logicExpr, ( calculation ? 'string' : 'boolean' ), path );
+
+                        // After checking that the non-constraint expression is valid, check for self-references.
+                        // Make an exception for a calculate="." as it does no harm.
+                        if ( logicName !== 'constraint' && !( logicName === 'calculate' && logicExpr.trim() === '.' ) ) {
+                            if ( xform.hasSelfReference( logicExpr, path ) ) {
+                                throw new Error( 'refers to itself' );
+                                //errors.push( `${friendlyLogicName} formula for "${nodeName}" refers to itself` );
+                            }
+                        }
+                        // TODO: check for cyclic dependencies between calculations, e.g. triangular calculation dependencies
                     } catch ( e ) {
-                        let friendlyLogicName = calculation ? 'calculation' : logicName;
-                        friendlyLogicName = friendlyLogicName[ 0 ].toUpperCase() + friendlyLogicName.substring( 1 );
                         errors.push( `${friendlyLogicName} formula for "${nodeName}": ${e}` );
                     }
+
                 }
             } );
 

src/xform.js

@@ -121,6 +121,29 @@ class XForm {
         }
     }
 
+    /**
+     * A function that performs a basic check for references to a path or self-as-dot.
+     * Note that this could easily be circumvented e.g. with a triangular dependency cycle 
+     * between nodes with expressions or using paths with predicates.
+     * It is a start of detecting obvious errors.
+     * 
+     * @param {string} expr 
+     * @param {string} selfPath 
+     */
+    hasSelfReference( expr, selfPath ) {
+        if ( !expr || !selfPath ) {
+            throw new Error( 'hasSelfReference function requires 2 parameters', expr, selfPath );
+        }
+        const self = this.enketoEvaluate( selfPath, 'node' );
+
+        return this._extractNodeReferences( expr )
+            .some( path => {
+                // The path could return muliple nodes, and self cannot be one of them.
+                const nodes = this.enketoEvaluate( path, 'nodes', selfPath );
+                return nodes.includes( self );
+            } );
+    }
+
     checkStructure( warnings, errors ) {
         const rootEl = this.doc.documentElement;
         const rootElNodeName = rootEl.nodeName;
@@ -411,6 +434,33 @@ class XForm {
         return parts.join( ', ' ).replace( /\.\s*,/g, ',' );
     }
 
+    /**
+     * [EXPERIMENTAL] Extracts node references.
+     * It excludes any references with predicates which is a big limitation but
+     * the goal is a better-safe-than-sorry approach to not have any false positives.
+     * 
+     * @param {string} expr 
+     */
+    _extractNodeReferences( expr ) {
+        return expr
+            // functions
+            .replace( /[a-z-:]+\(/g, ' ' )
+            .replace( /\)/g, ' ' )
+            .replace( /,/g, ' ' )
+            // * character when used for multiplication only
+            .replace( /(?<!\/)\*/g, ' ' )
+            // other operators (- character only when used for deduction)
+            .replace( /(\+| -|\|| and | or | mod | div |!=|=|<=|>=|<|>)/g, ' ' )
+            // remaining brackets that were not part of function calls
+            .replace( /(\(|\))/g, ' ' )
+            .split( /\s+/ )
+            // filter out empty strings, string literals, numbers
+            // Note this also filters out the path-as-string literal argument in jr:choice-name but who cares?
+            .filter( n => n && !( /".*"/.test( n ) ) && !( /'.*'/.test( n ) ) && isNaN( n ) )
+            // filter out anything with predicates as it is too difficult
+            .filter( n => !( /(\[|\])/.test( n ) ) );
+    }
+
 }
 
 module.exports = {

test/spec/xform.spec.js

@@ -137,7 +137,18 @@ describe( 'XForm', () => {
             expect( arrContains( result.errors, /deprecated/ ) ).to.equal( false );
             expect( resultOc.errors.length ).to.equal( ISSUES - 2 );
         } );
-
     } );
 
+    describe( 'with disallowed self-referencing', () => {
+        // Unit tests are in xpath.spec.js
+        const xf = loadXForm( 'self-reference.xml' );
+        const result = validator.validate( xf );
+
+        it( 'outputs errors for dissallowed self-referencing', () => {
+            expect( result.errors.length ).to.equal( 2 );
+            expect( arrContains( result.errors, /Calculation formula for "calc1".*refers to itself/i ) ).to.equal( true );
+            expect( arrContains( result.errors, /Relevant formula for "rel".*refers to itself/i ) ).to.equal( true );
+        } );
+    } )
+
 } );

test/spec/xpath.spec.js

@@ -11,6 +11,7 @@ const loadXForm = filename => fs.readFileSync( path.join( process.cwd(), 'test/x
 describe( 'XPath expressions', () => {
 
     const xf = new XForm( loadXForm( 'model-only.xml' ) );
+    xf.parseModel();
 
     describe( 'with function calls with an insufficient number of parameters', () => {
 
@@ -70,6 +71,32 @@ describe( 'XPath expressions', () => {
 
     } );
 
+
+    describe( 'with expressions that refer to self when not allowed', () => {
+        // There is an integration test in xform.spec.js
+        const FULL_PATH_TO_SELF = '/data/a';
+        [
+            '. + 1',
+            `${FULL_PATH_TO_SELF} + 1`,
+            'string-length(.)',
+            `string-length(${FULL_PATH_TO_SELF})`,
+            '../a + 1',
+            'string-length(../a)',
+            '.',
+            ' .',
+            '../*',
+            `weighted-checklist(${FULL_PATH_TO_SELF}, 9, /thedata/somenodes/*, /thedata/someweights/*)`,
+            'concat(/thedata/somenodes/*, sum(/data/*))',
+            `concat(/thedata/somenodes/*, sum(/data/b)) + 1 *${FULL_PATH_TO_SELF}`,
+            `something -${FULL_PATH_TO_SELF} *5`,
+        ].forEach( expr => {
+            it( `should be detected: ${expr}`, () => {
+                expect( xf.hasSelfReference( expr, FULL_PATH_TO_SELF ) ).to.equal( true );
+            } );
+        } )
+
+    } );
+
     describe( 'with instance() calls', () => {
 
         it( 'should throw an error message if instance does not exist in the form', () => {
@@ -134,6 +161,7 @@ describe( 'XPath expressions', () => {
 describe( 'XPath expressions (in custom OpenClinica evaluator)', () => {
 
     const xf = new XForm( loadXForm( 'model-only.xml' ), { openclinica: true } );
+    xf.parseModel();
 
     describe( 'with comment-status() calls', () => {
         it( 'should not throw an error message', () => {

test/xform/self-reference.xml

@@ -0,0 +1,29 @@
+<?xml version="1.0"?>
+<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:ev="http://www.w3.org/2001/xml-events" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:orx="http://openrosa.org/xforms" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
+  <h:head>
+    <h:title>skeleton</h:title>
+    <model>
+      <instance>
+        <data id="snapshot_xml">
+          <calc1>1</calc1>
+          <cons/>
+          <calc2/>
+          <rel/>
+          <answer_is_calc/>
+          <meta>
+            <instanceID/>
+          </meta>
+        </data>
+      </instance>
+      <bind calculate="( /data/calc1  + 1)" nodeset="/data/calc1" type="string"/>
+      <bind constraint="/data/cons > 1" nodeset="/data/cons"  type="string"/>
+      <bind calculate="." nodeset="/data/calc2"  type="string"/>
+      <bind relevant="/data/rel = '' " nodeset="/data/rel" type="string"/>
+      <bind nodeset="/data/answer_is_calc" readonly="true()" type="string"/>
+    </model>
+  </h:head>
+  <h:body>
+    <input ref="/data/answer_is_calc">
+      <label>answer is... <output value=" /data/calc1 "/></label></input>
+  </h:body>
+</h:html>