Fixes/html entities in version (#1693)

* Fixes getodk/central#1470: decode html entities in form version + fix _versionSplicer

* handle the case when version value is either delimited by single quote or no quote + more test data

Author: sadiqkhoja

lib/data/schema.js

@@ -21,7 +21,7 @@ const { always, equals, last, map } = require('ramda');
 const ptr = last; // just rename to make it more relevant to our context.
 const hparser = require('htmlparser2');
 const { parse } = require('csv-parse/sync');
-const { decodeXML } = require('entities');
+const { decodeXML, encodeXML } = require('entities');
 const semverSatisfies = require('semver/functions/satisfies');
 const Option = require('../util/option');
 const Problem = require('../util/problem');
@@ -542,18 +542,10 @@ const _versionSplicer = (replace) => (xml, insert) => new Promise((pass, fail) =
       if ((stripNamespacesFromPath(name) === 'version') && (stack.length) === 4 &&
         (stack[0] === 'html') && (stack[1] === 'head') && (stack[2] === 'model') &&
         (stack[3] === 'instance')) {
-        // okay, we have found the thing we are looking for.
-        // idx points at the position of the closing "
-        //
-        // TODO: we cheat here and reference the hp2 internal tokenizer index to find
-        // out where the attribute actually is. the parser startIndex and endIndex point
-        // at the whitespace preceding the tag until the tag is closed. obviously this is
-        // pretty bad but i don't see a more robust solution right now.
-        const idx = parser.tokenizer.index;
+        const newValue = encodeXML(replace ? insert : `${value}${insert}`);
+        const result = `${xml.slice(0, parser.startIndex)}${name}="${newValue}"${xml.slice(parser.endIndex)}`;
         parser.reset();
-        return replace
-          ? pass(`${xml.slice(0, idx - value.length)}${insert}${xml.slice(idx)}`)
-          : pass(`${xml.slice(0, idx)}${insert}${xml.slice(idx)}`);
+        return pass(result);
       }
     },
     // n.b. opentag happens AFTER all the attributes for that tag have been emitted!
@@ -565,7 +557,7 @@ const _versionSplicer = (replace) => (xml, insert) => new Promise((pass, fail) =
 
         const idx = _findSplicePoint(xml, parser.endIndex);
         parser.reset(); // stop parsing.
-        return pass(`${xml.slice(0, idx)} version="${insert}"${xml.slice(idx)}`);
+        return pass(`${xml.slice(0, idx)} version="${encodeXML(insert)}"${xml.slice(idx)}`);
       }
     },
     onclosetag: () => {

lib/model/frames/form.js

@@ -122,7 +122,7 @@ class Form extends Frame.define(
       const xmlFormId = idText.map(blankStringToNull)
         .orThrow(Problem.user.missingParameter({ field: 'formId' }));
 
-      const version = versionText.orElse('');
+      const version = versionText.map(decodeXML).orElse('');
       const name = nameText.map(decodeXML).orNull();
       const key = pubKey.map((k) => new Key({ public: k }));
       return new Form.Partial({ xmlFormId, name }, { def: new Form.Def({ version, name }), key });

test/integration/api/forms/draft.js

@@ -1936,6 +1936,17 @@ describe('api: /projects/:id/forms (drafts)', () => {
                   });
               })))));
 
+      it('should update version even if version provided in query parameter has special chars', testService((service) =>
+        service.login('alice', (asAlice) =>
+          asAlice.post('/v1/projects/1/forms/simple/draft')
+            .expect(200)
+            .then(() => asAlice.post('/v1/projects/1/forms/simple/draft/publish?version=x" version%3D"y')
+              .expect(200)
+              .then(() => asAlice.get('/v1/projects/1/forms/simple')
+                .then(({ body }) => {
+                  body.version.should.eql('x" version="y');
+                }))))));
+
       it('should log the correct def ids in the form audit log', testService(async (service, { Forms }) => {
         const asAlice = await service.login('alice');
 

test/integration/api/forms/forms.js

@@ -92,6 +92,19 @@ describe('api: /projects/:id/forms (create, read, update)', () => {
             body.hash.should.equal('07ed8a51cc3f6472b7dfdc14c2005861');
           }))));
 
+    it('should decode html entities in the value of form version - getodk/central#1470', testService((service) =>
+      service.login('alice', (asAlice) =>
+        asAlice.post('/v1/projects/1/forms')
+          .send(testData.forms.simple2.replace('2.1', '<{}>'))
+          .set('Content-Type', 'application/xml')
+          .expect(200)
+          .then(({ body }) => {
+            body.should.be.a.Form();
+            body.name.should.equal('Simple 2');
+            body.version.should.equal('<{}>');
+            body.hash.should.equal('232ffd287a8eaef0103ad2386dafa44e');
+          }))));
+
     it('should reject if form id ends in .xml', testService((service) =>
       service.login('alice', (asAlice) =>
         asAlice.post('/v1/projects/1/forms')

test/unit/data/schema.js

@@ -2020,6 +2020,53 @@ describe('form schema', () => {
   </h:body>
 </h:html>`)));
 
+    const arabicWord = 'صادق';
+    [
+      /* eslint-disable no-multi-spaces, key-spacing */
+      { versionAttribute: '',                         suffix: 'test',       expected: 'test' },
+      { versionAttribute: 'version=""',               suffix: 'test',       expected: 'test' },
+      { versionAttribute: 'version="first"',          suffix: '',           expected: 'first' },
+      { versionAttribute: 'version=\'first\'',        suffix: 'test',       expected: 'firsttest' },
+      { versionAttribute: 'version="2.1"',            suffix: 'test',       expected: '2.1test' },
+      { versionAttribute: 'version = "final"',        suffix: ' (working)', expected: 'final (working)' },
+      { versionAttribute: 'version=1',                suffix: '.0',         expected: '1.0' },
+      { versionAttribute: 'version=1.0',              suffix: '.1',         expected: '1.0.1' },
+      { versionAttribute: 'version=1',                suffix: '_final',     expected: '1_final' },
+      { versionAttribute: 'version="john\'s copy"',   suffix: '_final',     expected: 'john&apos;s copy_final' },
+      { versionAttribute: 'version="final"',          suffix: ' "working"', expected: 'final &quot;working&quot;' },
+      { versionAttribute: 'version="emoji 🙃"',       suffix: ' twice 🙃',  expected: 'emoji &#x1f643; twice &#x1f643;' },
+      { versionAttribute: `version="${arabicWord}"`,  suffix: arabicWord,   expected: '&#x635;&#x627;&#x62f;&#x642;&#x635;&#x627;&#x62f;&#x642;' },
+      {
+        versionAttribute: 'version="!@#$%^&amp;*()_+=-`~[]\\{}|&quot;:&apos;&lt;&gt;?/.,"',
+        suffix: '!@#$%^&*()_+=-`~[]\\{}|":\'<>?/.,',
+        expected: '!@#$%^&amp;*()_+=-`~[]\\{}|&quot;:&apos;&lt;&gt;?/.,!@#$%^&amp;*()_+=-`~[]\\{}|&quot;:&apos;&lt;&gt;?/.,'
+      },
+      /* eslint-enable no-multi-spaces, key-spacing */
+    ].forEach(({ versionAttribute, suffix, expected }) => {
+      it(`should have ${expected} version`, () =>
+        addVersionSuffix(testData.forms.simple2.replace('version="2.1"', versionAttribute), suffix)
+          .then(result => {
+            // we always return version with "" delimiter
+            result.match(/(version="(.*)")/)[2].should.be.equal(expected);
+          })
+      );
+    });
+
+
+    it('should suffix an existing version attribute even if it uses single quote to delimit version text', () =>
+      addVersionSuffix(testData.forms.simple2.replace('version="2.1"', "version='2.1\"final\"'"), 'testtest')
+        .then(result => {
+          result.includes('version="2.1&quot;final&quot;testtest"').should.be.true();
+        })
+    );
+
+    it('should suffix an existing version attribute even if it uses single quote to delimit version text', () =>
+      addVersionSuffix(testData.forms.simple2.replace('version="2.1"', 'version=1.0'), '.1')
+        .then(result => {
+          result.includes('version="1.0.1"').should.be.true();
+        })
+    );
+
     it('should suffix an existing namespaced version attribute', () =>
       addVersionSuffix(testData.forms.simple2.replace('version', 'orx:version'), 'testtest').then((result) => result.should.equal(`<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:jr="http://openrosa.org/javarosa">
   <h:head>
@@ -2156,6 +2203,44 @@ describe('form schema', () => {
     </input>
   </h:body>
 </h:html>`)));
+
+    const arabicWord = 'صادق';
+    [
+      /* eslint-disable no-multi-spaces, key-spacing */
+      { versionAttribute: '',                         newVersion: 'test',         expected: 'test' },
+      { versionAttribute: 'version=""',               newVersion: 'test',         expected: 'test' },
+      { versionAttribute: 'version="first"',          newVersion: '',             expected: '' },
+      { versionAttribute: 'version=\'first\'',        newVersion: 'test',         expected: 'test' },
+      { versionAttribute: 'version="2.1"',            newVersion: 'test',         expected: 'test' },
+      { versionAttribute: 'version = "final"',        newVersion: '(working)',    expected: '(working)' },
+      { versionAttribute: 'version=1',                newVersion: '1.0',          expected: '1.0' },
+      { versionAttribute: 'version=1.0',              newVersion: '2.0',          expected: '2.0' },
+      { versionAttribute: 'version=1',                newVersion: 'final',        expected: 'final' },
+      { versionAttribute: 'version="john\'s copy"',   newVersion: 'jane\'s copy', expected: 'jane&apos;s copy' },
+      { versionAttribute: 'version="final"',          newVersion: '"working"',    expected: '&quot;working&quot;' },
+      { versionAttribute: 'version="emoji 🙃"',       newVersion: '🙃 🙃',        expected: '&#x1f643; &#x1f643;' },
+      { versionAttribute: `version="sadiq"`,          newVersion: arabicWord,     expected: '&#x635;&#x627;&#x62f;&#x642;' },
+      {
+        // getodk/central#1470
+        versionAttribute: 'version="!@#$%^&amp;*()_+=-`~[]\\{}|&quot;:&apos;&lt;&gt;?/.,"',
+        newVersion: 'clean',
+        expected: 'clean'
+      },
+      {
+        versionAttribute: 'version="clean"',
+        newVersion: '!@#$%^&*()_+=-`~[]\\{}|":\'<>?/.,',
+        expected: '!@#$%^&amp;*()_+=-`~[]\\{}|&quot;:&apos;&lt;&gt;?/.,'
+      },
+      /* eslint-enable no-multi-spaces, key-spacing */
+    ].forEach(({ versionAttribute, newVersion, expected }) => {
+      it(`should have ${expected} version`, () =>
+        setVersion(testData.forms.simple2.replace('version="2.1"', versionAttribute), newVersion)
+          .then(result => {
+            // we always return version with "" delimiter
+            result.match(/(version="(.*)")/)[2].should.be.equal(expected);
+          })
+      );
+    });
   });
 
   describe('updateEntityForm', () => {