get not returning array object about refinerycms-settings HOT 21 CLOSED

More Detail, showing that it works correctly UNTIL setting is edited in admin/dashboard, after which returns STRING not ARRAY.

> sidelist = %w(recent_posts recent_news widgets)
 => ["recent_posts", "recent_news", "widgets"] 
> sidelist = ::Refinery::Setting.set(:gardenia_parts_sidebar, sidelist)
 => ["recent_posts", "recent_news", "widgets"] 
> sidelist = ::Refinery::Setting.get(:gardenia_parts_sidebar)
 => ["recent_posts", "recent_news", "widgets"] 

but after going in to the admin/dashboard, and editing the setting (by adding another entry)

> sidelist = ::Refinery::Setting.get(:gardenia_parts_sidebar)
 => "---\r\n- recent_posts\r\n- recent_news\r\n- widgets\r\n- wild_blue\r\n" 

now a string is returned; culprit appears to be editing the setting!

from refinerycms-settings.

I think I solve the problem, I tested it on hash and arrays.
Right now You can edit Your yaml structure from admin panel and all will be stored correct.

from refinerycms-settings.

Should this ticket be reopened, since the commit was reverted?

from refinerycms-settings.

We can bypass this with YAML.load but this tests are red with this patch:
https://github.com/refinery/refinerycms-settings/blob/master/spec/models/refinery/setting_spec.rb#L19
https://github.com/refinery/refinerycms-settings/blob/master/spec/models/refinery/setting_spec.rb#L25

from refinerycms-settings.

I wonder, as a matter of good security, if we should switch this project to use safe_yaml?

from refinerycms-settings.

Sure we can and I had same idea. But what with those two tests? I'll test it @parndt :)

from refinerycms-settings.

We could force strings to have quotes somehow but the situation seems to work with:

> require 'yaml'
 => true
> YAML::load YAML::dump(" whitespace \n @keram")
 => " whitespace \n @keram" 

So I guess we're not really doing this under the hood.

from refinerycms-settings.

@parndt but this will not work with this:

> YAML.load(YAML.dump("---\n- side_body\n- recent_posts\n- recent_news\n- midgets\n"))
 => "---\n- side_body\n- recent_posts\n- recent_news\n- midgets\n"

from refinerycms-settings.

Right. I wasn't sure that it'd work for anything else at all really.

from refinerycms-settings.

OK, all problems can be solved with YAML.load and by quoting:

YAML.load('"@keram"')
> "@keram"
YAML.load('" ryba "')
> " ryba "

Tested with standard and safe yaml.

Since we are using YAML for serialization, we have to respect it's specs.

http://www.yaml.org/spec/1.2/spec.html

Below example 5.9 -> @ problem.
Below example 7.8 -> whitespace problem.

from refinerycms-settings.

Seems fine, as long as we load using the safe flag.

from refinerycms-settings.

@parndt I don't think it is good idea to force safe_yaml, because it is using safe flag when safe is not disabled with false. But we can use safe flag for non-safe yaml. It is not causing any errors. And we can write in readme it is recommended to use safe_yaml in Gemfile.

from refinerycms-settings.

Since we dropped 1.8.x support in refinery/refinerycms@48a693a I wonder if we should bring back 937b512. Is YAML.parse(setting.value).to_ruby safe to use?

from refinerycms-settings.

Probably!

On Tuesday, 4 June at 8:56 AM Uģis Ozols wrote:

Since we dropped 1.8.x support in refinery/refinerycms@48a693a I wonder if we should bring back 937b512. Is YAML.parse(setting.value).to_ruby safe to use?

---

Reply to this email directly or view it on GitHub:
#6 (comment)

from refinerycms-settings.

Should I implement my plan?

from refinerycms-settings.

@simi what was your plan?

from refinerycms-settings.

@ugisozols #6 (comment)

from refinerycms-settings.

How is it determined if passed in yaml is safe or not? How do we specify safe flag?

from refinerycms-settings.

I think it is enough to mention this in readme. To use safe_yaml gem if you want. Including safe_yaml gem can change behaviour for whole app, that's reason why I don't like it turned on by default.

from refinerycms-settings.

Correct me if I'm wrong but I'm pretty sure that by using YAML.load we will introduce a security hole because we're going to pass user input to it. WDYT?

from refinerycms-settings.

@simi I like your plan

from refinerycms-settings.