-
Notifications
You must be signed in to change notification settings - Fork 418
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
fix: limit overly permissive regex range #949
Conversation
This fragment of code deals with letters so it should not match additional characters.
I'll check if it's not breaking bpmn-js, but should be safe to merge. |
Testing on bpmn-js via https://github.com/bpmn-io/bpmn-js/actions/runs/11815595868 |
I had to skip types test as they are apparently not generated for install from commit. This is the new run: https://github.com/bpmn-io/bpmn-js/actions/runs/11815660568 |
A manual smoke test did not reveal any issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose we add a dedcated test case for this, i.e. verify our replace strategy. Not 100% sold on whether this is actually a bug worth fixing.
For this to ever be an issue we'd need to get elements
from untrusted input.
I'll add a test case for this util. |
Test cases added via 06db3c7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one, thanks!
Proposed Changes
This fragment of code deals with letters so it should not match additional characters.
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/sr
toolCloses {LINK_TO_ISSUE}
orRelated to {LINK_TO_ISSUE}