Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
MEP32: matplotlibrc/mplstyle with Python syntax.#9528
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
jklymak commentedOct 22, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Cool - I think I prefer the straight python syntax. How does that look to the user in their code where they call the style sheet? Same as before? |
Indeed, I did not mention that point (nothing changes for the end user). Edited accordingly. |
a1dff3f
to43cab58
CompareMilestoning as 3.0, a) because I think this would be great, and popping to the top of everyone's queue, and b) because I would be surprised if it was accepted for 2.2. |
I also like plain python for configuration. I ran into a speed bump, but got over it, when doing something like this. See the |
Thanks for putting this together! I am coming around to this being the right thing to do (despite my grumbling). A third option for the full-python implementation is to look for modules with a function defapply_style(rcparams)->None: ... and have This would also allow for some interesting import hook implementations... An other option would be to make rcparams a |
Ping@anntzer - I personally think this would be a great feature for 3.0.... (once you are done expunging any py2-isms that have been driving you nuts). |
This is basically dead on arrival as far as I am concerned. I have spokento a few other people in the private sector who have to put their systemsthrough security review, and they all agreed that matplotlib loadingarbitrary, unsanitized, code upon import, with no ability to exclude thatoperation or force a plain config approach, would fail security review.This is particularly problematic because matplotlib is a dependency to somany other projects, people may not even be aware that their system isvulnerable.The argument that "ipython does it, why can't we?" is misleading. ipythonis intended to be an end-user component, with its design and implicationsall up front. You don't accidentally have ipython be a dependency becauseit was a dependency of something else a few layers deep. matplotlib is usedas both an end-user component *and* as an API library. Its use is almost asprevalent as numpy. Imagine if numpy executed a config file every time itwas imported!If this were to go forward, serious work would need to go into its design,particularly for sanitizing the input and constraining its effects. I doubtthat the value proposition is there versus using a proper parser, severalof which exists and could be adapted to our use. …On Wed, Mar 7, 2018 at 4:38 PM, Jody Klymak ***@***.***> wrote: Ping@anntzer <https://github.com/anntzer> - I personally think this would be a great feature for 3.0.... (once you are done expunging any py2-isms that have been driving you nuts). — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#9528 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-LTdfWOy_TZ3Vr7kmhfkQOwIo2h-ks5tcFNlgaJpZM4QCEt2> . |
Good point. I do wonder about MPL arbitrarily loading a config file, without the opportunity to turn that convenience off, or even better to have it turned off by default. I think it makes sense that downstream packages should never load any matplotlibrc file. |
anntzer commentedMar 8, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I am not particularly interested in relitigating this question (although I will state once again that 1) did you know that np.load can also execute arbitrary code? 2) the "proper parser" proposed has been in a state of vaporware for quite a while), but given that both@tacaswell and@efiring consider that this is a good approach, I'll let them present their thoughts (if any) on the security implications. As for the implementation strategy I think I quite like@tacaswell's suggestion of requiring an |
which is why I don't use np.load() in my production code! We alreadycovered this. The point is that I can choose to not use np.load(). I canchoose to not use ipython. But if matplotlib goes down this route, it wouldhappen at import time, and can't be avoided. …On Wed, Mar 7, 2018 at 9:16 PM, Antony Lee ***@***.***> wrote: I am not particularly interested in relitigating this question (although I will still say that 1) did you know that np.load can also execute arbitrary code? 2) the "proper parser" proposed has been in a state of vaporware for quite a while), but given that both@tacaswell <https://github.com/tacaswell> and@efiring <https://github.com/efiring> consider that this is a good approach, I'll let them present their thoughts (if any) on the security implications. As for the implementation strategy I think I quite like@tacaswell <https://github.com/tacaswell>'s suggestion of requiring an apply_style hook. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#9528 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-D4YXLPGMF05OQgji7688Mg8zM6-ks5tcJR6gaJpZM4QCEt2> . |
Just to throw in my 2 cents: From what I see none of the arguments against In my personal opinion |
I guess I'm +0 on using yaml (if it is indeed possible to define custom type loaders), I don't like the language but it's not too bad andmost importantly at least it has a spec... |
Thanks for pointing out ruamel! I hadn't noticed it before! …On Thu, Mar 8, 2018 at 3:08 AM, Antony Lee ***@***.***> wrote: I guess I'm +0 on using yaml (if it is indeed possible to define custom type loaders), I don't like the language but it's not too bad and *most importantly* at least it has a spec... — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#9528 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-CihlfOEKXAX5zZzB0dSjqWjwCU4ks5tcOcRgaJpZM4QCEt2> . |
Given how it shows up in my conda updates, I think ruamel is used by conda for parsing its config. I agree some with@WeatherGod here--as a library, we need to tread differently than an application. In theory, the same level of permissions required to modify the config would be used to run the script (baring misconfiguration), so no security risk. On the other hand, we're literally talking about adding a hook to trigger arbitrary code execution, which screams out CVE to me (as irrational as that might be). (I think I've changed my feeling from in the past.) |
Thanks for pointing that out. At that point I'm genuinely curious about what is the threat model that we're talking about avoiding. As in, can you present any way in which such a patch (say e.g. matplotlib executes ~/.config/matplotlibrc.py on import, just to pick a concrete case) actually makes things more dangerous? Note also that youcan opt out: just set the MATPLOTLIBRC environment variable to os.devnull (/dev/null) before importing matplotlib. (Well, if that doesn't work, then there should be a patch that makes it work.) |
I'm not a security expert, nor do I want to pretend to be, so I feel really uncomfortable trying to decide what's "safe". There's also a difference between what's actually safe and what our user community would perceive as safe. |
anntzer commentedMar 8, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I guess it's good that the discussion restarted there, because I got to think about the issue again and am going to renege on my "uninterest in relitigating it" mentioned above. Let's, again, use a concrete proposal as basis of discussion: at import time, matplotlib tries to import a module named matplotlibrc(.py) from the normal config path (possibly modified by MATPLOTLIBRC), and tries to call the apply_style(rc) function defined there. It is possible to disable this feature (for example) by setting MATPLOTLIBRC to os.devnull. Note that neither The fear appears to be that that module can be modified by an attacker or an oblivious user to execute arbitrary code, e.g. From this I conclude that we (and nearly all other Python packages that have external dependencies) are already vulnerable to arbitrary code execution (in that model), and can essentially do nothing about it. Edit: for improved impact, replace six.py by some stdlib module name, of course. |
Just my 2 cents, I think this conversation should have input from somebody with experience in security issues who can vet the proposal. Security vulnerabilities are hard to spot and, as suggested in this thread, can be a deal-breaker for some groups. I'd feel more comfortable moving forward on this if a trusted voice said that it was OK from a security standpoint. |
h-vetinari commentedApr 30, 2021
Sorry for resurrecting an old issue, but I just saw it milestoned for 3.5.0 and had a look. I was wondering iftoml had been considered (it's not part of the list of formats in the alternatives). It basically combines (IMO) the best of both worlds between YAML & JSON, and results in a very readable format (and is getting ever more widespread use throughPEP518, cargo, etc.). Since this allows easily defining arrays etc., and has a fully specified & verifiablegrammar, that might also help with having to |
I don't think PEP518 is a good argument here; PEP518 explicitly chose to introduce a new format rather than using Python literals (https://www.python.org/dev/peps/pep-0518/#python-literals) because they expect build systems to be written in other languages than Python (note that this is theonly point against them), whereas we certainly don't expect matplotlibrc being read by anyone other than Matplotlib. |
h-vetinari commentedMay 1, 2021
I'm not saying it's a complete answer (e.g. I haven't understood the |
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
I think the idea is well-advertised now and whether the issue is open or closed won't change much to the discussion. |
As a remark, strictYAML may be an improvement over the very general and complex YAML standard.https://github.com/crdoconnor/strictyaml |
Uh oh!
There was an error while loading.Please reload this page.
See proposed MEP text in the PR. The rendered version is available by clicking on
view
at the top right of the "files changed" tab.I had this written for a while but was hoping to push this a bit later (I don't really have the time to work on it now).#6157 (comment) made me consider at least publishing my current thoughts.
attn @matplotlib/developers