-
Notifications
You must be signed in to change notification settings - Fork 46
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
Bug 1349596: Validate JEXL on the server instead of the client. #629
Conversation
FWIW I ran some of the more complex filters from prod against this and they all validated fine. |
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.
# Add mock transforms for validation | ||
jexl.add_transform('date', lambda x: x) | ||
jexl.add_transform('stableSample', lambda x: x) | ||
jexl.add_transform('bucketSample', lambda x: x) |
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.
This isn't very satisfying, but I don't know how we can do better. Maybe we can put a link to the docs that say what transforms need to be available?
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 skimmed the pyjexl code. It looks good, except for one small issue that was fixed in. 0.1.3.
r+wc
@@ -120,3 +120,6 @@ django-csp==3.2 \ | |||
html5lib==1.0b10 \ | |||
--hash=sha256:08a3efc117a4fc8c82c3c6d10d6f58ae266428d57ed50258a1466d2cd88de745 \ | |||
--hash=sha256:0d5fd54d5b2b79b876007a70c033a4023577768d18022c15681c00561432a0f9 | |||
pyjexl==0.1.2 \ |
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.
This should be pyjexl 0.1.3 now.
Test failures are unrelated and fixed by #632. |
Ok, can you rebase this to pick up the changes in #632? Then this should be ready to merge. |
Validating JEXL on the client isn't ideal, because the JEXL library doesn't have a public API for checking if a statement is valid without evaluating it, which means certain valid expressions won't pass validation if they reference data in the context that we don't provide in the admin interface. PyJEXL is a Python library that parses and evaluates JEXL, and it includes a validation API that is more suitable for our validation.
bc2604d
to
9b99989
Compare
Rebased! |
Validating JEXL on the client isn't ideal, because the JEXL library doesn't
have a public API for checking if a statement is valid without evaluating
it, which means certain valid expressions won't pass validation if they
reference data in the context that we don't provide in the admin interface.
PyJEXL is a Python library that parses and evaluates JEXL, and it includes
a validation API that is more suitable for our validation.