Translate directive should only render sanitized HTML when postCompile is off

[marklagendijk]

The translate directive renders the text as HTML instead of text. Example:

{
  MY_LABEL: 'This is<br /> an example.'
}

<div translate="MY_LABEL"><div>

Is rendered as:

This is
an example.

This is a nice feature. However, when I found out about this feature I tested what would happen if I would put a script tag in the label. Turns out that it is rendered and the containing script executed. Example:

{
  MY_LABEL: 'This is<br /> an example.<script type="text/javascript">alert("Wait! What?");</script>'
}

This is a serious security issue. The solution would be to make it work in the same way as ngBindHtml by using the $sanitize service. This will add a dependency on ngSanitize, but is the only way to do this properly.

[knalli]

Valid point. That's why we have http://angular-translate.github.io/docs/#/guide/19_security -- original cause is backwards compatibility to AJS 1.2 (no ngSanitize).

[marklagendijk]

Is it possible to configure it in such way that it uses $sanitize instead of doing .text? I do want to be able to use HTML in my labels, but don't want to introduce this security issue.

Also I don't agree with the current strategy of: let's keep this serious security issue for backwards compatibility. I would rather see a major version bump with this listed as a breaking change, and maybe the possibility to turn it off. I'm afraid that otherwise many people will overlook this in the documentation, like I did.

[knalli]

Also I don't agree with the current strategy

Ehm, this was introduced while AJS 1.2 was available as stable. There is no "strategy", it's simply not possible. Afaik there was no simple way to inject a non existing module.. ?

I would rather see a major version bump with this listed as a breaking change, and maybe the possibility to turn it off. I'm afraid that otherwise many people will overlook this in the documentation, like I did.

Again, a valid point. Default security should be as high as possible (https://www.owasp.org/index.php/Establish_secure_defaults). A 3.0.0 will enable at least the escaped way (or even more) just like the documentation already said. Unfortunately, this project does need support by the community while we have a bunch of issues to do.

[marklagendijk]

Oh, ok. So you are open to fixing this properly, which means introducing a breaking change? I could see if I can find the time to implement this.

Actually, I looked in the docs and it turns out that $sanitize has been available since 1.0. Using it has a drawback, though. Since ngSanitize is released as a separate module, people do have to include it manually. However, I do think this is worth is, because I agree with you that default security should be as high as possible.

I hope you didn't take my comments as critique of the highly appreciated work you guys are doing on this module. I only meant to provide some constructive feedback.

Btw, I quickly looked through the code. It looks like that when you specify 'escape' only the values are sanitized, and not the translation texts themselves. I think it would be best to also sanitize the texts.

[knalli]

You can make a proposal, but we will not make such a breaking change in a minor release. By definition this have to be 3.0.0.

[knalli]

Said this, I'm open for a breaking change release (aka 3.0.0), but this will probably take some time...

[knalli]

fyi, I've opened #995 starting the topic about breaking changes...

[marklagendijk]

Thanks! I will look into this issue and make a proposal about how we should fix it. Hopefully I will also find the time to actually implement it, as soon as we agree on how it should be done.

[knalli]

The actual plan was to extract the "strategies" as dedicated "plugins" (easiest way would be a simple function via config or even a module aka component/service) allowing to share them with all kinds of interpolators. Said this, this was the idea. But was not realized until now.

[marklagendijk]

I am willing to implement this. My proposal would be to do it as follows:

1. Refactor the sanitazation strategies in $translateDefaultInterpolation so they are usable for both sanitizing the values and sanitizing the whole result.
2. Implement sanitizing the whole result.
3. Use the $sanitize strategy as the default for the whole result. And either the escape strategy or nothing as the value sanitizing strategy. It might make sense to use no value sanitizing by default, because the whole result will be sanitized by $sanitize. This way you would have a both secure and powerful default.
4. Allow setting both strategies. I would say that besides strings which refer to a known strategy, we should also allow to directly pass a (custom) sanitizing function.

@knalli what do you think? Does this sound like a good approach?

[knalli]

@marklagendijk Thank you!

1. Definitely.
2. Do you mean a new strategy? Then yes, too.
3. I don't get this. Another strategy yes, a new global default not before next major (as already in angular-translate 3.0.0 (proposal roadmap, not final) #995).
4. What you mean with both?

[marklagendijk]

The idea I had was to have two kind of strategies:

1. sanitizeValueStrategy
2. sanitizeTranslationStrategy

The idea of the sanitizeTranslationStrategy is that it would be applied on the whole result, after interpolation.

Was your idea to use one strategy both? This would be possible. We could just call the same sanitize function (belonging to the currently selected strategy) in both cases. This function should then check whether it got passed an object (-> params) or an string (-> whole translation), and act accordingly.

This would also make it a non-breaking change, so it could already be released as part of 2.x. We could just add several strategies:

1. escaped: keep this exactly the same, for backwards compatability. Warn in the docs against using it. The name should be deprecated.
2. escapedParameters: new name for the old escaped strategy.
3. sanitizedTranslation: sanitizes the whole result, does nothing with the parameters.
4. sanitizedTranslationEscapedParameters: sanitizes the whole result, and escapes the parameters (useful if the parameters come from user input, and you want to forbid HTML).
5. escapedTranslation: escapes the whole result, does nothing with the parameters (because they are already escaped as part of the whole result).

In 3.0 we would change to either sanitizedTranslation or sanitizedTranslationEscapedParameters as the default strategy.

@knalli let me know what you think. If you have suggestions for better names they are also welcome.

Edit: by injecting $sanitize from inside the strategy function, it would not break any applications which do not reference ngSanitize.
Edit2: what branch should I create the pull request against? Canary?

[knalli]

Let's play with a demo set.

Templates

1. This is <b>only an example</b> with a {{param}}!
2. <b>Hello</b> {{param}}!

Strategy: 0. none

Obviously the bad one. No transformations.

Strategy: 1. escaped

Everything will be escaped. Actually the safest one, but obviously not always what you want.

1. This is <b>only an example with a <span onclick="alert('XSS')">xss attack</span>! => This is <b>only</b> an example with a &lt;span onclick="alert('XSS')"&gt;xss attack&lt;/span&gt;!
2. <b>Hello</b> <script>alert("XSS by User");</script>! => <b>Hello</b> &lt;script&gt;alert("XSS by User");&lt;/script&gt;!

Strategy: 2. escapedParameters

Same as escaped?

Strategy: 5. escapedTranslation

1. This is <b>only an example with a <span onclick="alert('XSS')">xss attack</span>! => This is <b>only</b> an example with a <span onclick="alert('XSS')">xss attack</span>!
2. <b>Hello</b> <script>alert("XSS by User");</script>! => <b>Hello</b> <script>alert("XSS by User");</script>!

---

And now the additional two. I dont get why you say only 2 strategies. A small bit didn't get into my mind..

[marklagendijk]

Your examples are correct. Only the sentence in stragey 1. escaped is incorrect: "Everything will be escaped. Actually the safest one, but obviously not always what you want.". This is true of Strategy: 5. escapedTranslation, but not of escapedParameters.

I will work out the other two strategies:

Templates

1. This is an example with a <b>malicious</b> translation string <span onclick="alert('XSS')">xss attack</span>!
2. This is an example of a <b>malicious</b> param {{ param }}

Stragey 3. sanitizedTranslation

This strategy calls $sanitize on the whole translation, after interpolation.

1. This is an example with a <b>malicious</b> translation string <span onclick="alert('XSS')">xss attack</span> => This is an example with a <b>malicious</b> translation string <span>xss attack</span>
2. This is an example of a <b>malicious</b> param <span onclick="alert('XSS')">xss attack</span>! =>This is an example of a <b>malicious</b> param <span>xss attack</span>!

Stragey 4. sanitizedTranslationEscapedParameters

This strategy escapes parameters using the escapedParameters strategy. The strategy also calls $sanitize on the whole translation, after interpolation.

1. This is an example with a <b>malicious</b> translation string <span onclick="alert('XSS')">xss attack</span> => This is an example with a <b>malicious</b> translation string <span>xss attack</span>
2. This is an example of a <b>malicious</b> param <span onclick="alert('XSS')">xss attack</span>! =>This is an example of a <b>malicious</b> param &lt;span onclick="alert('XSS')"&gt;xss attack&lt;/span&gt;!

[knalli]

Your examples are correct. Only the sentence in stragey 1. escaped is incorrect: "Everything will be escaped. Actually the safest one, but obviously not always what you want.". This is true of Strategy: 5. escapedTranslation, but not of escapedParameters.

Written before I had seen the fine difference between escapedParameters and -Translations. However, this is only valid in case you are not trusting the translation definition content itself. ;)

---

Hm, okay, you are chaining the transformations actually.

Perhaps we should split this down into

• none
• escapedParameters
• escaped (aka escapedTranslation)
• sanitizedParameters (<< useful?)
• sanitized (aka sanitizedTranslationEscapedParameters w/ escaped)

And the we define a chaining strategy like one of these

• none
• escaped
• escaped, sanitized
• escapedParameters, sanitized
• and so one

wdyt?

[marklagendijk]

Sounds good. Please see this gist. As a proposed implementation.

The only problem with your proposed naming is that 'escaped' now has a new meaning. In the current version of angular-translate it means 'escapedParameters'. Do you think it is acceptable to change that without bumping the major version?

Edit: after some refactoring iterations the code in the proposal is now quite clean. Please let me know what you think.

[marklagendijk]

I think we could solve the backwards compatibility issue by using slightly different names:

• sanitize
• escape
• escapeParameters
• sanitizeParameters
• sanitize-escapeParameters

This way we could do:

// Backwards compatibility for deprecated 'escaped'
sanitizeValueStrategies.escaped = sanitizeValueStrategies.escapeParameters;

I must say that I slightly prefer this naming :).

[knalli]

@marklagendijk : Yes, this looks good! I'm with you.

In the final implementation I would think it is better to split the strategies (dedicated component functions?) and no hardcoded combinations (except a default one in the future).

What happens in an AJS 1.2 environment with this one? https://gist.github.com/marklagendijk/55b98aba74f053752c19#file-default-interpolation-js-L78

I guess this will break everything (at least the running context). It would be okay to catch it gracefully and fallback to something else. We can cover this in the tests as they are running in each context (AJS 1.2, AJS 1.3, AJS 1.4).

[marklagendijk]

@knalli I was planning to refactor the strategies to its own $translateSanitization service (one service for all strategies). This service would supply the following functionality:

1. Configuring the current sanitation strategy. The interpolation services should not be responsible for this.
2. Providing the different sanitation strategies.
3. Allowing adding sanitation strategies via a provider.
4. Supplying a sanitize function, which would be used by the different interpolation services.

Do you agree with this approach?

As for $sanitize, it has been available since Angular 1.x, but always as part of the separate ngSanitize module. This means that you have to add a reference to add a script reference to angular-sanitize.js and a dependency on the ngSanitize module.
By using the $injector I made sure that it only fails if you actually are trying to use a sanitation strategy which uses $sanitize. I think that in these cases you should get a fatal error, because the code is unable to do what you requested. We should, however, add a more friendly error message, explaining how it can be fixed (either by adding ngSanitize, or by switching to another sanitation strategy).

[knalli]

It's okay when the strategy won't works unless they have import ngSanitize. But I will only avoid a breaking change (i.e. new dependency in core).

A good error message, but a working (with fallback) is okay.

[knalli]

Should be covered when it will be ensured the lines are not running without explicit activation of the strategy in question.

[marklagendijk]

@knalli can you please review this commit?

I took the following approach:

• Created a $translateSanitization provider which contains all the strategies and maintains the currently selected strategy.
• Refactored the interpolators to use $translateSanitization
• Refactored $translate to use the $translateSanitizationProvider to set the strategy.

I think this approach is quite powerfull:

1. In the most common use case you just select the strategy you want via $translate.useSanitizeValueStrategy, just as it was before.
2. If you want to use a custom strategy you can just pass the strategy function to $translate.useSanitizeValueStrategy.
3. If you want even more control you can inject the $translateSanitizationProvider to change the strategies via $translateSanitizationProvider.strategies.

Let me know what you think. If you agree with this approach I will finish it, and make tests etc.

[knalli]

I'll have a look later for details.

[marklagendijk]

Ok, thanks! I added support for specifying a chain of strategies in this commit.

[knalli]

@marklagendijk Well done!

I've made some remarks (actually, you had already seen this).

Looks good. For the PR please:

• style guide regarding things like spaces, line breaks and so (in this proposal not being required)
• some tests would be cool
•  squash please

And finally: I want to add a warning about not using a safe strategy (via $log.debug?) unless useStrategy has been called at least once. I can take care of this after merge if you want.

[marklagendijk]

@knalli see pull request #1014.

[Heshyo]

I had an issue with the sanitization as I was passing the whole $scope as parameter (the string to translate was dynamic and could contain different variables, so it felt easier/cleaner to just pass the scope than creating a brand new object with only the needed properties). When trying to sanitize the whole $scope, I end up with a stackoverflow exception.

I guess it means we're trying to sanitize everything from the passed object, while maybe we could try to sanitize only the necessary properties, ie the ones that are actually used by the current string to translate.

[knalli]

Well, both strategies try to either escape or sanitize all existing values in the object. That is not good, eventually $scope as a param is not a good solution.

I guess it means we're trying to sanitize everything from the passed object, while maybe we could try to sanitize only the necessary properties, ie the ones that are actually used by the current string to translate.

... which would means we have to pre-parse the string looking for each interpolation expression. We don't do this at the moment, as we are using standard AJS interpolation expressions. I'll file a new issue, feel free to contribute.

Closing this here, we had introduced sanitization.