ValentijnvdBeek
commented
5 years ago Hi Palmer,
Thanks for your contributions to Clay, I appreciate the time and effort you spent on it. However, most of the pull requests address bugs that already have been fixed but haven't landed in the master branch yet due to issues within the project (described fully in #57, but boils down to project owner being gone and me not knowing what to do). This one, for instance, fixes #55 which already includes a solution and another fixes #38 where the bug reporter, @agg23, also contributed a patch. I can't really blame you for this, it is both confusing and my entirely my fault, but please make sure to either base your work on the development branch (called 'porcelain') or contribute to my up-to-date soft hard fork at ValentijnvdBeek/clay.
Anyways, to the pull request itself, the behaviour of Gentoo is kind ridiculous in the sense that we aren't loading any YaML from the internet at all so the security vulnerability doesn't affect us. Suffice to say that the moment an attacker can access and freely modify a file, all is already lost. The two commits are related however since safe_dump and safe_load address the same thing, mainly which subset of Python it can process. On later on versions of PyYaML using the dump procedure will give a warning. Porcelain carries a fix for the load and my fork has both fixed.
Anyways, long story short, good work on the commits. They look good and mergeable accept for the fact that they, by happenstance, already have been fixed. I'll look at the others in short order but do keep in mind that I am on vacation in France for the upcoming three weeks so it'll take me some time to get back to you.
Cheers,
Valentijn
These two changes are only related in that I found them at the same time. The change to yaml.load() fixes a crash on Gentoo-based systems. The change to yaml.dump() doesn't fix an error, I just stumbled across it. More information is in the patches.