search

facebookarchive / facebook-for-magento

A first-party extension plugin built for Magento. This plugin will install a pixel on your site, upload your products into a catalog for use in FB ads, and (optionally) auto-create an FB shop with your products.
https://www.facebook.com/business/help/532749253576163
84 stars 57 forks source link

wrap json encoded content with " instead of ' #52

Closed MattDunbar closed 6 years ago

MattDunbar commented 6 years ago

JSON encode doesn't escape single quotes as JSON uses double quotes.

MattDunbar commented 6 years ago

This fix isn't actually correct, although the issue is real.

I fixed it locally by doing str_replace ' -> \', and keeping the json_encode wrapped in single quotes.

mpalasis commented 6 years ago

agree with @MattDunbar this would likely break my site, but could you provide an example string wherein there are single quotes and they are indeed not escaped? sample from my 'main': "Admin > Default > Admin":"0","Main Website > afoipalasi Store > \u0395\u03bb\u03bb\u03b7\u03bd\u03b9\u03ba\u03ac ":"1",

If single quote escaping is needed, JSON_HEX_APOS could be used as a parameter. See json_encode reference

MattDunbar commented 6 years ago

We had a store name with a single quote and it was rendered directly without escaping.

On Thu, Apr 12, 2018 at 2:04 AM, Makis Palasis notifications@github.com wrote:

agree with @MattDunbar https://github.com/MattDunbar this would likely break my site, but could you provide an example string wherein there are single quotes and they are indeed not escaped? sample from my 'main': "Admin > Default > Admin":"0","Main Website > afoipalasi Store > \u0395\u03bb\u03bb\u03b7\u03bd\u03b9\u03ba\u03ac ":"1",

If single quote escaping is needed, JSON_HEX_APOS could be used as a parameter. See json_encode http://php.net/manual/en/function.json-encode.php reference

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/facebookincubator/facebook-for-magento/pull/52#issuecomment-380562039, or mute the thread https://github.com/notifications/unsubscribe-auth/AAbMs2e5MuegNJ6p6vr3hq7jACKC1kTuks5tnlPXgaJpZM4TP4Kf .

mpalasis commented 6 years ago

methinks, change this: ,stores: '<?php echo json_encode($this->getStores()) ?>' to: ,stores: '<?php echo json_encode($this->getStores(),JSON_HEX_APOS) ?>'

Good catch btw, you might wanna offer the PR for this as it might be possible single quotes can be valid in some store names in the wild as apostrophes in some locales. Although, any weary (see: paranoid) site admin would/should advise against putting apostrophes in data strings.

MattDunbar commented 6 years ago

That should do it.

On Thu, Apr 12, 2018 at 2:10 AM, Makis Palasis notifications@github.com wrote:

methinks, change this: ,stores: '<?php echo json_encode($this->getStores()) ?>' to: ,stores: '<?php echo json_encode($this->getStores(),JSON_HEX_APOS) ?>'

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/facebookincubator/facebook-for-magento/pull/52#issuecomment-380563599, or mute the thread https://github.com/notifications/unsubscribe-auth/AAbMsxx5RGZa8J3OsWSKT-cik8S0i2X8ks5tnlUzgaJpZM4TP4Kf .