search

noahmorrison / chevron

A Python implementation of mustache
MIT License
480 stars 52 forks source link

Defaulting to yaml.FullLoader with hidden cli arg to resolve GH-78 #86

Closed binokkio closed 3 years ago

binokkio commented 3 years ago

I considered a couple of options but this seemed to be a good solution for GH-78. It preserves support for yaml data files when pyyaml is installed and defaults to the yaml.FullLoader for parsing. A cli arg allows the cli user to specify a different loader. This cli arg is hidden since it is quite an edge case and support for yaml data files itself is also not mentioned.

I look forward to receiving feedback and updating the PR accordingly.

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.02%) to 99.209% when pulling 45a08f41aec50cb65b99f5884b2a9842eb65d5ff on binokkio:fix/yaml-loader_GH-78 into 4bc5648a4f7b9f07e7d7246d2131ba72379552e8 on noahmorrison:master.

samuelcolvin commented 3 years ago

I thinking defaulting to FullLoader which is known to have vulnerabilities is a mistake.

I don't use the CLI so so it's not a problem personally, but I would think it was much safer to default to SafeLoader.

binokkio commented 3 years ago

@samuelcolvin I choose the FullLoader because it is what the yaml load function uses when no loader was specified. This way the output is exactly the same as before this PR. I wanted to prevent breaking things for people who rely on the current behavior while giving the option to choose the safer loader. Happy to change to the SafeLoader if changing that behavior is no concern.

samuelcolvin commented 3 years ago

Well, the reason PyYAML changed the default behaviour was that they assumed (reasonably I think) that very few people intentionally chose FullLoader (let alone UnsafeLoader).

FullLoader has some known vulnerabilities, so I think the pragmatic choice would be to default to SafeLoader, highlight this in the change log, and let people use another loader if they require it.

But, I'm not the primary maintainer of this library, so that's just my opinion.

noahmorrison commented 3 years ago

Yeah I'd prefer if the default was SafeLoader. Using the FullLoader was definitely a mistake I made when writing this originally. While you are right that this would be a breaking change, I'm pretty sure it would only break CLI usage and only if someone was actually using the unsafe features of the FullLoader.

If you could change the default to SafeLoader I'll merge this in. Thanks for writing it up!

binokkio commented 3 years ago

Thanks! SafeLoader it is, definitely the better default but not a change I felt comfortable making without your input.