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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require 'safe_yaml/load' instead of 'safe_yaml' #1982

Merged
merged 2 commits into from Jan 31, 2014

Conversation

Projects
None yet
6 participants
@dtao
Contributor

dtao commented Jan 24, 2014

Originally I intended for SafeYAML to be used by application developers, and so it made sense to patch YAML.load to protect an application against YAML-related deserialization exploits that could creep in anywhere, either in the application code or in some library dependency. Over time (see dtao/safe_yaml#47) it's become clear that there's another totally legit use case, which is a library that wants to deserialize YAML safely but doesn't want to do any crazy patching of global modules.

I think jekyll fits this use case, which means it probably ought to just require "safe_yaml/load" and not monkeypatch the YAML module with potential ripple effects. See for example dtao/safe_yaml#53, which is an issue caused for a project that's apparently using both jekyll and sidekiq.

dtao added some commits Jan 24, 2014

added a test that YAML.load doesn't get clobbered
I THINK this is a good idea? I considered multiple approaches to testing this;
what I like about this dumb way (just try to deserialize a symbol) is that it's
nice and simple.

@ghost ghost assigned mattr- Jan 24, 2014

@parkr

This comment has been minimized.

Member

parkr commented Jan 24, 2014

Thanks for investigating, @dtao, and thanks for the PR!

Would love to ensure this doesn't cause any security holes (dependencies, etc). Will try to run through the ringer and get back to you. :)

@benbalter

This comment has been minimized.

Contributor

benbalter commented Jan 24, 2014

@gregose, @mastahyeti low priority, but would love a 👍 or 👎 on the proposed changed to SafeYAML if you have a free cycle.

@mastahyeti

This comment has been minimized.

Contributor

mastahyeti commented Jan 27, 2014

Looks like the way we're using it now, SafeYAML.load ends up getting called anyway. I don't see a problem with switching this. I do think that Jekyll is normally run as a standalone application though and that we don't need to make the switch.

@dtao

This comment has been minimized.

Contributor

dtao commented Jan 27, 2014

I do think that Jekyll is normally run as a standalone application though [...]

That's what I would have thought too; but as I mention in my initial comment, there do appear to be some applications that have a dependency on jekyll (for whatever reason), and these apps are affected if they or any of their dependencies rely on YAML.load deserializing arbitrary objects by default.

@parkr

This comment has been minimized.

Member

parkr commented Jan 27, 2014

If there's no problem with this from a security standpoint, and this patch fixes the use of Jekyll in some instances, I'm 👍 on merging this.

@mattr-

This comment has been minimized.

Member

mattr- commented Jan 27, 2014

👍 from me as well.

mattr- added a commit that referenced this pull request Jan 31, 2014

@mattr- mattr- merged commit 5edb4c6 into jekyll:master Jan 31, 2014

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Jan 31, 2014

@dtao dtao referenced this pull request Aug 15, 2014

Closed

Do not patch YAML by default #61

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.