Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validation for simple XPaths in form config #486

Merged
merged 12 commits into from Jul 12, 2022
Merged

Conversation

jkuester
Copy link
Contributor

@jkuester jkuester commented Jul 1, 2022

Description

Adds support for validating simple XPath expressions in form XML files. Simple XPaths, (ones that only describe a full path (either absolute or relative) to a node) in forms should always refer to a node that actually exists in the form (otherwise they would never actually result in a value). XPaths that fail this check are almost certainly a bug.

More complex XPath's (that might have conditional logic that depends on the actual data in the form) are not evaluated.

Note that I have only been able to find one "false positive" case (where an error is flagged when it should not be). This case requires the XPath expression to include:

  • Multiple string literals that are quoted using different quotes (single quotes vs double quotes)
    • (note that XPath does not support escape characters so if one string literal contains a quote character (single or double) then the string literal, itself, must be created with the other type of quote.)
  • The former string literal contains a value that technically is an invalid simple XPath expression (but should not be flagged as an error since it is in a string).
  • A latter string literal contains a single (or odd number) quote character (either single quote or double quote) that is the same type of quote as the characters used to quote the string literal containing the XPath expression

For example, concat('/Users/joe/Desktop/myfile.txt', "Joe's File") will cause the validation to fail saying that /Users/joe/Desktop/myFile.txt is an invalid XPath expression because the quote in "Joe's" confuses the regex into thinking that '/Users/joe/Desktop/myfile.txt' is not actually in a string literal. This seems like a pretty extreme edge case and not one that I am terribly worried about hitting. However, if any forms do run into this, it is worth noting that it can easily be fixed by updating the string literal containing the simple XPath to just use the other type of quote: concat("/Users/joe/Desktop/myfile.txt", "Joe's File").

Closes #479

Code review items

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or integration tests where appropriate
  • Backwards compatible: Works with existing data and configuration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@@ -11,7 +11,7 @@
"eslint": "eslint 'src/**/*.js' test/*.js 'test/**/*.js'",
"docker-start-couchdb": "npm run docker-stop-couchdb && docker run -d -p 6984:5984 --rm --name cht-conf-couchdb couchdb:2.3.1 && sh test/scripts/wait_for_response_code.sh 6984 200 CouchDB",
"docker-stop-couchdb": "docker stop cht-conf-couchdb || true",
"test": "npm run eslint && npm run docker-start-couchdb && npm run clean && mkdir -p build/test && cp -r test/data build/test/data && cd build/test && nyc --reporter=html mocha --forbid-only ../../test/**/*.spec.js && cd ../.. && npm run docker-stop-couchdb",
"test": "npm run eslint && npm run docker-start-couchdb && npm run clean && mkdir -p build/test && cp -r test/data build/test/data && cd build/test && nyc --reporter=html mocha --forbid-only \"../../test/**/*.spec.js\" && cd ../.. && npm run docker-stop-couchdb",
Copy link
Contributor Author

@jkuester jkuester Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I needed these extra quotes in here to get mocha to run the tests nested down in test/lib/validation/form

@@ -38,6 +38,7 @@
"@hapi/joi": "^16.1.8",
"@medic/translation-checker": "^1.0.1",
"@parcel/watcher": "^2.0.5",
"@xmldom/xmldom": "^0.8.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, @xmldom/xmldom is just the new version of xmldom.

Copy link
Member

@m5r m5r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, LGTM!

src/lib/forms-utils.js Show resolved Hide resolved
@jkuester jkuester merged commit 9df4a63 into main Jul 12, 2022
@jkuester jkuester deleted the 479_validate_xpath branch July 12, 2022 13:46
@medic-ci
Copy link
Collaborator

🎉 This PR is included in version 3.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when invalid xPath paths in form
3 participants