Potential vulnerability with yaml.loader

[FabioRosado]

Description

I was listening to Talk Python to Me podcast - the one where they talk about Python vulnerabilities and things that we shouldn't be doing in Python. One of the things that they mention was using yaml.load instead of yaml.safe_load.

I know that when I wrote the bit of code so we can use !include I use yaml.loader so perhaps this should be changed to yaml.safe_load. One thing I didn't understand was if the yaml.loader is now set as the default safe_load or not. (Mostly due to this issue).

I tried to swap yaml.load for yaml.safe_load but tox complained and both the environmental variables and the !include tests are failing.

Should I dig deeper and try to make the safe_load work?

Also, this article in hackernoon might be interesting for reference about this issue - 10 common security gotchas in Python and how to avoid them

[jacobtomlinson]

Funnily enough I was looking into this yesterday!

The recent PRs have been failed due to there being an open CVE for pyyaml. From looking into it I think this vulnerability has been ackowledged for a while but when using the latest version of pyyaml with python 3.7 you may experience the vulnerability.

They are planning on releasing a new version of pyyaml which addresses this vulnerability. As this issue will affect all existing versions of opsdroid I think we are ok to go ahead and merge PRs which have failed due to this.

I totally agree that we should explore yaml.safe_load. If you have the time to dig deeper that would be great!

[FabioRosado]

Yeah, I was a bit confused due to that PR that said it implements safe_load as default although the documentation still tells you to not use load and use safe_load instead. For some reason, the tests are failing I will look into it further and try to figure out why they are failing 👍

[jacobtomlinson]

Getting #574 merged is the priority at the moment. If you could look into it that would be great.

[FabioRosado]

Closed as it got solved in 803