search

formatjs / handlebars-intl

Handlebars helpers for internationalization.
http://formatjs.io/handlebars/
Other
265 stars 28 forks source link

[Enhancement] formatMessage to accept SafeString arguments #59

Closed jasonmit closed 9 years ago

jasonmit commented 9 years ago

This is based on a conversation I had with @ericf where he wants to support the use case of passing arguments which are SafeStrings.

Handlebars.registerHelper('strong', function (input) {
  return new Handlebars.SafeString('<strong>' + input + '</strong>');
});
{{formatMessage 'Today is {date}' date=(strong (formatDate 'date'))}}

Outputs: Today is 1/20/2014

{{formatMessage '<em>{date}</em>' date=(strong (formatDate 'date'))}}

Outputs (escapes the em tags still): &lt;em&gt;1/20/2014&lt;/em&gt;

Live example: http://jsbin.com/pekoxipuyo/1/edit?html,output

yahoocla commented 9 years ago

CLA is valid!

ericf commented 9 years ago

@jasonmit looking good!

I'm finding the two variables isHtml and hasSafeString hard to get my head around when thinking of all the possible flows through the code. Can you think of how to possibly simply this so it's easier to follow, or add code comments?

jasonmit commented 9 years ago

@ericf comments addressed and removed isHtml. I added a comment blurb as to what hasSafeString is doing. Let me know if you're looking for more detail.

Also added a jsbin to demo the functionality. http://jsbin.com/pekoxipuyo/1/edit?html,output

ericf commented 9 years ago

@jasonmit I see you added a unit test already, but could you fill out the unit tests so that we have full branch code coverage in {{formatMessage}} and {{formatHTMLMessage}} helper functions? I just want to be sure we don't have any security holes since these changes deal with escaping.

ericf commented 9 years ago

I've thought through this for over an hour and it's really hard to fully understand the all the possible flows through the code (not your fault). This opens up weird behavior around double-escaping when the message is not a string, and also removes the protections that were part of the contract with {{formatMessage}}.

I'm going to close this for now, but don't delete the branch. No one has actually been asking for this feature, I thought it would have been a nice to have, but I'm concerned about the added complexity and change in the security contract of these changes.

jasonmit commented 9 years ago

Sounds good, I'd be happy to revisit any ideas you have around this in the future :+1: