Command line argument for custom validators #202
Conversation
…w `-i` argument.
|
This pull request introduces 1 alert when merging 09b7479 into d7f9657 - view on LGTM.com new alerts:
|
| if not os.path.isfile(include): | ||
| raise ValueError("Python include file '{}' not found.".format(include)) | ||
| from importlib.machinery import SourceFileLoader | ||
| SourceFileLoader(os.path.basename(include), include).load_module() |
There was a problem hiding this comment.
Since this step allows users of the yamale CLI to load arbitrary python code, the README.md warning may need to be broadened.
|
Thanks for your contribution! We usually like to have a discussion in an issue first to ensure not too much coding happens in the wrong direction. This seems pretty contained though, so not a problem. Just to keep in mind for the future. I'll be out for a week, so it'll take a while for me to get to this. Initially, on the surface, it does look good though. |
|
If this is accepted/merged, the new flag should be documented in the README, probably along with a section that links to the example custom validators and a shows how to use it. |
|
Just going to quote from #166 (comment) here, since that issue is the mirror of this one (custom validators inlined in schemas), and the concerns mostly apply:
|
|
I appreciate the reminder of the previous concerns. I believe this is a little different since it's not embedding the validators in the schema itself, but it's in a separate python file that is pointed to within the command line options. This makes it far less dangerous as well, and shouldn't require an extra warning in the README. However, I've only looked at this for 5 minutes, so please let me know if I missed something. :) |
|
Ah yep, same, just did an initial read and the non-technical concerns popped in my head, will need to come back and look deeper. I consume yamale only as a library, so the concern is not personal. I'm not sure what promises/guarantees the CLI makes (is there an implicit "only the schema flag is dangerous" due to the warning?). From a user POV it's "I give flag A file X, and flag B file Y" so warning about A (schema) but not B (includes) when both can lead to undefined behavior (schema via ast exploits, includes via it being loaded as a python module) seems odd. |
|
The danger comes from offering Yamale as a service. If that service is accepting a schema/data pair and offering back a result, there are concerns that processing uncontrolled schema would leave the service open to malicious attacks. When limited to command line, the caller of the I understand your concern, and I don't mind putting a line in the README saying to "ensure the imported validators come from trusted sources" or something similar. |
|
I wanted to do a little check in here. Work is really busy at the moment. This hasn't been forgotten, but will take a bit for me to get to....my apologies. |
First of all, thank you for this great tool. It is a crucial part of our build pipeline to maintain code quality and therefore incredibly useful to our project.
As Yamale already features a modular design, able to be extended by custom validators, my pull request aims to allow this using command line arguments.
There are no breaking changes in my pull request and I wrote a test that completes successfully for python 3.6, 3.8 and (with a newer pytest version) 3.10 on my system.