search

Automattic / i18n-calypso

i18n JavaScript library on top of Jed originally used in Calypso
GNU General Public License v2.0
24 stars 19 forks source link

Safe JSON stringification for cache key #54

Closed marekhrabe closed 6 years ago

marekhrabe commented 6 years ago

This hotfixes the problem from https://github.com/Automattic/wp-calypso/issues/24674

…but also in general makes this function more bullet-proof as it's not considered very safe calling JSON.stringify on untrused data without a try/catch block.

frontdevde commented 6 years ago

👋 @akirk and @Tug added you as reviewers as suggested by GitHub. Also added @ramonjd who reported the issue. We have this issue as a card on our board and it'd be cool if we could move this one along. Thanks in advance for taking a look! 💯

Tug commented 6 years ago

PR looks good to me!

I think we could also solve it by disabling caching for translates with args in general. It makes some sense to me as it could help in cases where we have a dynamic translate with frequent changes. In this case the LRU cache would fill up quickly and it would bust out all other useful translate results.

cacheable = ! options.components && ! options.args;

A try catch here is still a good addition, so 👍 for shipping it!

ramonjd commented 6 years ago

Hope you don't mind. I've merged in light of @frontdevde 's comments.

ramonjd commented 6 years ago

Bumped version: https://github.com/Automattic/i18n-calypso/pull/62

Will create a corresponding PR in Calypso once version is bumped and tag created.

marekhrabe commented 6 years ago

Good! I was AFK last week so thanks for handling all this you two! :)

marekhrabe commented 6 years ago

There is much more to the general issue but this is at least patches the uncaught error that would crash Calypso. It's a start