diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a49a6f..a62cadc 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/package.json b/package.json index acc4eb0..42a30e4 100644 --- a/package.json +++ b/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" } -} +} \ No newline at end of file diff --git a/src/validator.js b/src/validator.js index 0eb4961..0ffc585 100644 --- a/src/validator.js +++ b/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}` ); } + } } ); diff --git a/src/xform.js b/src/xform.js index 6ea0abd..f7008e2 100644 --- a/src/xform.js +++ b/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, ' ' ) + // 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 = { diff --git a/test/spec/xform.spec.js b/test/spec/xform.spec.js index 5c16044..d851fe8 100644 --- a/test/spec/xform.spec.js +++ b/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 ); + } ); + } ) + } ); \ No newline at end of file diff --git a/test/spec/xpath.spec.js b/test/spec/xpath.spec.js index 52ab580..e780aa8 100644 --- a/test/spec/xpath.spec.js +++ b/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', () => { diff --git a/test/xform/self-reference.xml b/test/xform/self-reference.xml new file mode 100644 index 0000000..f11322f --- /dev/null +++ b/test/xform/self-reference.xml @@ -0,0 +1,29 @@ + + + + skeleton + + + + 1 + + + + + + + + + + + + + + + + + + + + +