dtao / safe_yaml

Parse YAML safely
MIT License
217 stars 63 forks source link

switch to a whitelist and exception raising #23

Closed codekitchen closed 11 years ago

codekitchen commented 11 years ago

So this ended up as a pretty huge change, I'm sure it still has some small things to clean up. The two major differences in the API are

Does this match with the direction you want to take the safe_yaml gem? I wouldn't have minded making the exception raising an option, but I didn't see a great way to do that while still allowing the normal YAML loading functionality to do the heavy lifting. If that doesn't meet the needs of your use case, I totally get it.

dtao commented 11 years ago

Whew, you weren't kidding! This is indeed a big change. I definitely appreciate what you're doing here; my main concern is the fundamental change in behavior (throwing an exception on encountering a non-whitelisted tag). I'm going to think about this and specifically investigate whether we can't somehow have it both ways here (use built-in deserialization on whitelisted tags but default to "safe" parsing) before moving forward. Either way, I hope this works for you on your own project for now!

codekitchen commented 11 years ago

Honestly for our use case, we feel that raising an exception is the right thing to do even if falling back to "safe" parsing is possible. But I understand that's maybe not going to be true of all use cases.

Oh I just realized it'd be better to make the exception a subclass of the normal YAML parse error exception classes.

dtao commented 11 years ago

So I ended up making a similarly massive change, but implemented it differently. I actually like your tag extractor implementation a lot, and may yet end up pulling it from this pull request. But what I set out to do as default behavior was to match SafeYAML's current functionality (basically, sanitize to "raw" form by default) plus the ability to whitelist tags. I also understand how in your case (and probably many other cases), it may be more desirable to simply raise an exception on encountering an unknown tag rather than silently sanitize it—for that reason I also added a :raise_on_unknown_tag option.

So I think what I have now provides roughly the same benefit as what you submitted here, plus a bit more. But I'd be curious to know your thoughts (I'm going to close this issue, though, since I think it doesn't make sense to merge this at this point). If you want to take SafeYAML in a different direction, that's totally fine too!