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

feat(stringvalidator): client provided regex #487

Merged
merged 3 commits into from
Aug 16, 2022

Conversation

mttrbrts
Copy link
Member

@mttrbrts mttrbrts commented Aug 16, 2022

This PR reverts the changes introduced in #484 to adopt the RE2 engine. The engine has some constraints (no browser support, memory limits etc.), so I'd like to simplify Concerto, but allow consumers to supply their own engine where they can make own judgements on the tradeoffs.

Changes

  • Revert back to the default JavaScript RegExp engine by default for all platforms.
  • Add a new options parameter to the ModelManager constructor to allow a client to provide their own RegExp engine.
const XRegExp = require('xregexp')
XRegExp.install('astral');
const modelManager = new ModelManager({ regExp: XRegExp });

Flags

  • This change makes Concerto vulnerable to ReDoS attacks if client applications do not introduce further checks. This strategy is consistent with the approach that other validation libraries take (see AJV).

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to master from fork:branchname

Signed-off-by: Matt Roberts <[email protected]>
@mttrbrts mttrbrts merged commit 28984e4 into accordproject:master Aug 16, 2022
Copy link
Contributor

@sstone1 sstone1 left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

3 participants