rails icon indicating copy to clipboard operation
rails copied to clipboard

YAML serialization options

Open xjunior opened this issue 1 year ago • 9 comments

Summary

Allow setting YAML serialization options on a per attribute basis – along with application wide setting.

  • Permit YAML classes per attribute (serialize :content, yaml: { permitted_classes: [Symbol] })
  • Permit YAML unsafe load per attribute (serialize :content, yaml: { unsafe_load: true })

Other Information

Rails' versions 7.0.3.1, 6.1.6.1, 6.0.5.1, and 5.2.8.1 introduced application wide setting to allow unsafe YAML load, along with a global permitted classes setting. This PR intends to add such options on a per attribute basis, so updating to the new behavior can be done incrementally, while also providing more flexibility when allowing YAML to load classes.

xjunior avatar Jul 25 '22 19:07 xjunior

In case this is accepted, would it be fine to backport/release this to 5.x and 6.x?

xjunior avatar Jul 25 '22 19:07 xjunior

I'm having the same issue

fredericosilva-hotmart avatar Aug 01 '22 14:08 fredericosilva-hotmart

Having the same issue here too. Thanks @xjunior for this PR. You are shining like the sun rising.

felipebutcher avatar Aug 01 '22 14:08 felipebutcher

Had the same issue here. Thanks @xjunior for this PR. You saved the day here buddy.

uxpalladin avatar Aug 01 '22 15:08 uxpalladin

would it be fine to backport/release this to 5.x and 6.x?

No. We don't backport new features to stable releases.

rafaelfranca avatar Aug 05 '22 14:08 rafaelfranca

would it be fine to backport/release this to 5.x and 6.x?

No. We don't backport new features to stable releases.

I get it, but in this case, this was a security patch that introduced a feature creating problems on older releases. Wouldn't that be acceptable?

We have 30 models using the YAML serializer in the application I work. We're working on getting this app up to rails 6, then 6.1, and finally 7. Meanwhile, we've disabled safe loads – which is alright since it has always been unsafe.

We want to work toward enabling safe loads. Ideally, that could be done model by model, allowing specific classes for specific cases or sometimes allowing models to use unsafe load. We'd love to do that before we get to rails 7.

xjunior avatar Aug 05 '22 16:08 xjunior

For anyone with this issue, this script looks good to find what you serialize: https://gist.github.com/crawler/47a1e66ee2c2ea37f56f9c0c2aac071a

xjunior avatar Aug 05 '22 16:08 xjunior

Actually, even Rails 7.0 would not get this change. The security fix didn't introduce any problem on existing application. Existing application can ,on their own risk, disable the security fix or allow all the classes that they want to allow globally.

This isn't a bug fix for the security fix, which we could consider backporting. This is a new feature, not necessary to apply the security fix, neither to upgrade the applications to the latest version.

rafaelfranca avatar Aug 05 '22 17:08 rafaelfranca

@rafaelfranca I guess it makes sense. Thanks!

We're using the aforementioned script to figure out the classes so we can properly fix this CVE in our app. If you're running a rails 5, replace the pairs method with a hardcoded collection of Class/method pairs.

xjunior avatar Aug 08 '22 14:08 xjunior

Merged in 4a075537dc0592753718b0f2d264ba56297ee4d3. Thanks

rafaelfranca avatar Nov 23 '22 18:11 rafaelfranca