Skip to content
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

How do I load this without monkey patching? #47

Closed
rkh opened this issue Sep 13, 2013 · 9 comments
Closed

How do I load this without monkey patching? #47

rkh opened this issue Sep 13, 2013 · 9 comments

Comments

@rkh
Copy link

rkh commented Sep 13, 2013

This is currently unusable for libraries. :(

@dtao
Copy link
Owner

dtao commented Sep 16, 2013

Honest question: how do you plan on using this in a library?

Originally, my intention was for safe_yaml to be useful to application developers. The monkey patching--which mastahyeti introduced way back in the first pull request, and I've stuck with--makes it very useful in this context: all you do is add a dependency on safe_yaml, and automatically your application is safer wherever your code or any code calls YAML.load.

My guess is that you have a library and you want it to deserialize YAML somewhere, and you want to use YAML.safe_load for that. Is that correct? I guess I can see three possible approaches here:

  1. Remove the monkey patching from safe_yaml and require an extra step for it (like YAML.monkeypatch! or something less stupid). The reason I don't like this option is that it makes life just slightly harder for app devs, and I really like the fact that currently all this gem needs to work is a single require statement.
  2. Add an option to reverse the monkey patch. This would be similar to setting SafeYAML::OPTIONS[:default_mode] = :unsafe. This seems bad too, though, as you don't want to, as a library, potentially reverse the decision that an app developer already made.
  3. Split the gem in two: yaml_safe_load (or something like that), which adds the functionality of safe_yaml without touching YAML itself; and safe_yaml, which depends on yaml_safe_load and does the monkey patching.

The third is the most work, but I kinda think it's the best approach. Of course there could be a fourth option I'm not thinking of.

@rkh
Copy link
Author

rkh commented Oct 4, 2013

I wrote a library for parsing TAP. TAP can contain YAML and is in most cases untrusted input. I would like to be able to require something in my library so that I can load YAML safely but don't mess with the YAML.load method for anything else. That is, if safe_yaml has been loaded by the app using my library, it should still be safe, otherwise it should still be able to load any unsafe objects. Therefore option 2 would not work. I would also prefer 3. It doesn't need to be a separate gem, it could also be that I load safe_yaml/safe_load instead of safe_yaml or something.

@dtao
Copy link
Owner

dtao commented Oct 4, 2013

Ha, that's a good point. Pretty ridiculous that I didn't even think of that. It should be pretty easy to extract the safe_load functionality into a separate file; I will do that and try to get it pushed soon.

dtao added a commit that referenced this issue Oct 4, 2013
This is so you can require safe_yaml/load without monkey-patching
the YAML module if you want.
@dtao
Copy link
Owner

dtao commented Oct 4, 2013

OK, I've pushed a change that I think should address your needs. Try updating your gem spec to temporarily pull from HEAD and use require safe_yaml/load. Let me know if that works for you and/or if you see any major issues with this approach.

@fnichol
Copy link

fnichol commented Nov 22, 2013

I'm the same boat, using this for a library. Would love to push a release with this addition.

Would a confirmation of the behavior help a release of safe_yaml? Thanks @dtao!

@dtao
Copy link
Owner

dtao commented Dec 2, 2013

@fnichol Sorry for the delayed response. Yeah, if you have tried out the safe_yaml/load solution and it works for you, that'd be good to know. The build is currently failing and I've been dragging my feet on investigating; but once I square that away and make sure this approach works for at least one person besides me (!) I intend on releasing the next version.

@fnichol
Copy link

fnichol commented Dec 2, 2013

No problem, I'll attempt to give it a spin today or tomorrow to confirm. Thanks!

@dtao
Copy link
Owner

dtao commented Dec 27, 2013

OK I'm closing this since require "safe_yaml/load" is available now in SafeYAML 1.0. If anybody experiences any problems with that, let me know and I will reopen!

@dtao dtao closed this as completed Dec 27, 2013
@fnichol
Copy link

fnichol commented Dec 30, 2013

@dtao This is great and has been doing the trick for me, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants