[I18n] Compile html in angular directive by LeanidShutau · Pull Request #23684 · elastic/kibana

LeanidShutau · 2018-10-02T10:44:06Z

Resolves #23359

We need to compile html in i18n directive to be able to pass html with angular directive via values, for example

<p>
  <span
    i18n-id="test.test"
    i18n-default-message="I just check {justToCheck}"
    i18n-values="{ justToCheck: '<strong ng-click=&quot;checkDirectivesAreCompiled()&quot;>click me!</strong>' }"
  ></span>
</p>

It's not secure, so we need to sanitize or escape html in defeultMessage or in values, that are not supposed to contain potentially dangerous html code. One of the solutions is to use unsafe_ prefix in some values keys and sanitize/escape html in defaultMessage and all other values.

pavel06081991 · 2018-10-02T11:28:15Z

packages/kbn-i18n/src/angular/directive.ts

@azasypkin I believe now it is safe to compile html because:

we have agreed not to use tags in defaultMessage.

even if some vendor pass any tags, scripts or directive in defaultMessage for some language, before inserting defaultMessage to DOM we check passed string using $sanitize angular service which removes all unsafe tags like script and directives.
For example we have such text in defaultMessage:

<span>hello</span>world <im-dangerous-directive>I will destroy something</im-dangerous-directive> <script>document.write('ha-ha')</script>

after using $sanitize service we will get:

<span>hello</span>world

pavel06081991 · 2018-10-02T11:49:40Z

@azasypkin this merge request is necessary for us to be able to pass angular directives via values. Now we also can pass directives but they do not work because they need to be compiled.

To check that all works as expected you can:
in file src\core_plugins\kibana\public\dashboard\dashboard_app.html
replace for example this code:

<p class="kuiText kuiVerticalRhythm">
  Click the <a kbn-accessible-click class="kuiButton kuiButton--primary kuiButton--small" ng-click="showAddPanel()" aria-label="Add visualization" data-test-subj="emptyDashboardAddPanelButton">Add</a> button in the menu bar above to add a visualization to the dashboard. <br/>If you haven't set up any visualizations yet, <a class="kuiLink" href="#/visualize">visit the Visualize app</a> to create your first visualization.
</p>

with this code:

<p>
  <span i18n-id="test.test"
    i18n-default-message="I just check {justToCheck}"
    i18n-values="{ justToCheck: '<strong ng-click=&quot;checkDirectivesAreCompiled()&quot;>click me!</strong>' }">
  </span>
</p>

and in file src\core_plugins\kibana\public\dashboard\index.js
find line .when(DashboardConstants.CREATE_NEW_DASHBOARD_URL, { and make it look like this:

.when(DashboardConstants.CREATE_NEW_DASHBOARD_URL, {
  template: dashboardTemplate,
  controller: ($scope) => {
    $scope.checkDirectivesAreCompiled = () => {
      alert('I am working!');
    };
  },
  resolve: {

open dashboard page.
If you click on 'click me!' text and see alert window, then it works because angular directive ng-click now works when passed via values.

elasticmachine · 2018-10-02T12:23:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-10-02T13:31:55Z

ack: Looking into this one at the monent

pavel06081991

LGTM. Checked it works as expected.

elasticmachine · 2018-10-05T16:32:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

LeanidShutau · 2018-10-08T07:39:39Z

retest

elasticmachine · 2018-10-08T09:19:27Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…tml-in-i18n-directive

elasticmachine · 2018-10-08T13:34:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…tml-in-i18n-directive

elasticmachine · 2018-10-10T15:28:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…tml-in-i18n-directive

elasticmachine · 2018-10-16T16:16:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

LeanidShutau · 2018-10-17T07:53:43Z

retest

elasticmachine · 2018-10-17T09:33:25Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…tml-in-i18n-directive

elasticmachine · 2018-10-17T11:22:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-10-17T15:54:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-10-17T16:31:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

LeanidShutau · 2018-10-18T07:34:11Z

retest

elasticmachine · 2018-10-18T09:12:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…tml-in-i18n-directive

elasticmachine · 2018-10-18T16:15:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-10-22T12:55:09Z

packages/kbn-i18n/src/angular/directive.ts

        $scope.$watchCollection('values', () => {
-          setHtmlContent($element, $scope, i18n);
+          setHtmlContent($element, $scope, $sanitize, i18n);
+          $compile($element.contents() as any)($scope.$parent);


issue: I'm still afraid of this "global" compile.... with this even $sanitize'ed variable without usafe_ can be pretty harmful, imagine the case that looks like this (help link that is displayed somewhere else on this page will lead user to the attacker site):

<span i18n-id="kbn.management.someId" i18n-default-message="Some text that seems safe: {val}" i18n-values="{ val: '\{\{controller.helpLink=\'http://attacker-site/\';\'value\'\}\}' }"> </span>

Is there any way we can compile just unsafe_ pieces? Can you paste a couple of real use cases? If it's only about directives (e.g. <a ng-href\ng-click), we can probably mark all these elements with directive with some custom data-unsafe-i18n data attribute or class and then query only these things and compile.

I know it's becoming complex, but I feel uncomfortable with how it's currently. Basically we have 3 cases (sorted by popularity):

Simple default message + simple values, in this case we should just use element.text(i18n.translate(...)) - text will escape everything for us.

Simple default message + values with HTML (html_ prefix) - in this case we have to use element.html, so we should escape default message (let's try lodash.escape instead of manual solution, we have several lodash.* packages in Kibana already anyway) + escape ordinary values (without html_ prefix) + $sanitize html_ values

Simple default message + unsafe values (unsafe_ prefix_) - in this case we also have to use element.html, so again we should escape default message + escape ordinary values + $sanitize html_ values. And once we inserted them into DOM, we should call $compile for nodes with data-unsafe-i18n (or something like this).

Am I over-engineering this (or missing something) @spalger @LeanidShutau?

Is there any way we can compile just unsafe_ pieces? Can you paste a couple of real use cases? If it's only about directives (e.g. <a ng-href\ng-click), we can probably mark all these elements with directive with some custom data-unsafe-i18n data attribute or class and then query only these things and compile.

Good point. I believe we can do it. It looks like in this case we do not need to use unsafe_ prefix for values' property names.

I have to be honest, I don't have enough context to say if this is over-engineering or a comprehensive solution. Since I've been asked to review this and it seems like the overall direction is still in flux I'd really like to chat about what's going on here and how/why we're doing things this way. The PR description and the linked issue have basically no details and I just don't know enough to review this...

azasypkin · 2018-10-22T12:55:46Z

packages/kbn-i18n/src/angular/directive.ts

-      values: $scope.values,
-      defaultMessage: $scope.defaultMessage,
+      values,
+      defaultMessage: ($scope.defaultMessage || '').replace(/\</g, '&lt;').replace(/\>/g, '&gt;'),


question: why do we need || ''? I believe we shouldn't ever have empty default message?

LeanidShutau · 2018-11-01T11:03:41Z

We don't need angular directive compiling any more. $sanitize part was implemented in separate PR: #24830

Issue has been solved by refactoring:

<p class="kuiText kuiVerticalRhythm">
  <span
    i18n-id="kbn.dashboard.addVisualizationDescription1"
    i18n-default-message="Click the "
    i18n-context="Part of composite label kbn.dashboard.addVisualizationDescription1 + {link} + kbn.dashboard.addVisualizationDescription2"
  >
  </span>
  <a
    i18n-id="kbn.dashboard.addVisualizationLink"
    i18n-default-message="Add"
    kbn-accessible-click class="kuiButton kuiButton--primary kuiButton--small"
    ng-click="showAddPanel()"
    aria-label="{{ 'kbn.dashboard.addVisualizationLinkAria...' | i18n: { defaultMessage: 'Add visualization' }}"
    data-test-subj="emptyDashboardAddPanelButton">
  </a>
  <span
    i18n-id="kbn.dashboard.addVisualizationDescription2"
    i18n-default-message=" button in the menu bar above to add a visualization to the dashboard. {br}If you haven't set up any visualizations yet, {visitVisualizeAppLink} to create your first visualization."
    i18n-values="{
        br: '<br/>',
        visitVisualizeAppLink: '<a class=\'kuiLink\' href=\'#/visualize\'>' + visitVisualizeAppLinkText + '</a>'}"
    i18n-context="Part of composite label kbn.dashboard.addVisualizationDescription1 + {link} + kbn.dashboard.addVisualizationDescription2"
  >
  </span>
</p>

This PR can be reopened in the future, if needed

LeanidShutau added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Project:i18n v6.5.0 labels Oct 2, 2018

LeanidShutau self-assigned this Oct 2, 2018

LeanidShutau requested review from azasypkin and pavel06081991 October 2, 2018 10:44

pavel06081991 reviewed Oct 2, 2018

View reviewed changes

LeanidShutau added the review label Oct 2, 2018

LeanidShutau requested review from azasypkin and pavel06081991 and removed request for azasypkin October 4, 2018 07:18

pavel06081991 approved these changes Oct 4, 2018

View reviewed changes

Aliaksandr Yankouski and others added 2 commits October 5, 2018 17:47

add ability to compile child directives

8d14ffc

Add typings and update tests

6486b93

LeanidShutau force-pushed the feature/compile-html-in-i18n-directive branch from 2bc299e to 6486b93 Compare October 5, 2018 14:49

Merge remote-tracking branch 'upstream/master' into feature/compile-h…

0a8490e

…tml-in-i18n-directive

LeanidShutau mentioned this pull request Oct 8, 2018

[I18n] Translate Timelion #23880

Merged

This was referenced Oct 9, 2018

Translations for ui/vis #23694

Merged

Translations for Tag Cloud #23184

Closed

Merge remote-tracking branch 'upstream/master' into feature/compile-h…

71115a2

…tml-in-i18n-directive

This was referenced Oct 10, 2018

Translations for Table Vis plugin #23679

Merged

Translations for Region Map #23875

Merged

Merge remote-tracking branch 'upstream/master' into feature/compile-h…

c551b36

…tml-in-i18n-directive

LeanidShutau requested a review from azasypkin October 17, 2018 07:54

Merge remote-tracking branch 'upstream/master' into feature/compile-h…

ad5ba8c

…tml-in-i18n-directive

LeanidShutau added 2 commits October 17, 2018 17:13

Escape html tags in defaultMessage

de46c9b

Fix tests

61c4f2b

Merge remote-tracking branch 'upstream/master' into feature/compile-h…

2b441cc

…tml-in-i18n-directive

jinmu03 requested a review from spalger October 18, 2018 17:55

This was referenced Oct 22, 2018

Feature/dashboard translations #24328

Merged

Translate security/roles component #23984

Merged

azasypkin reviewed Oct 22, 2018

View reviewed changes

maryia-lapata added v6.6.0 and removed v6.5.0 labels Oct 24, 2018

jinmu03 requested a review from epixa October 28, 2018 05:02

LeanidShutau added discuss and removed review labels Oct 30, 2018

LeanidShutau mentioned this pull request Oct 30, 2018

[I18n] Sanitize angular directive message before inserting it to DOM #24830

Merged

LeanidShutau removed request for epixa and spalger November 1, 2018 10:55

LeanidShutau closed this Nov 1, 2018

azasypkin mentioned this pull request Nov 26, 2018

Angular i18n directives should treat as HTML only values that are marked as such #26173

Closed

Conversation

LeanidShutau commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavel06081991 Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel06081991 commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Oct 2, 2018

💚 Build Succeeded

Uh oh!

azasypkin commented Oct 2, 2018

Uh oh!

pavel06081991 left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 5, 2018

💔 Build Failed

Uh oh!

LeanidShutau commented Oct 8, 2018

Uh oh!

elasticmachine commented Oct 8, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Oct 8, 2018

💚 Build Succeeded

Uh oh!

elasticmachine commented Oct 10, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Oct 16, 2018

💔 Build Failed

Uh oh!

LeanidShutau commented Oct 17, 2018

Uh oh!

elasticmachine commented Oct 17, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Oct 17, 2018

💚 Build Succeeded

Uh oh!

elasticmachine commented Oct 17, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Oct 17, 2018

💔 Build Failed

Uh oh!

LeanidShutau commented Oct 18, 2018

Uh oh!

elasticmachine commented Oct 18, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Oct 18, 2018

💚 Build Succeeded

Uh oh!

azasypkin Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

pavel06081991 Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

spalger Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

azasypkin Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

LeanidShutau commented Nov 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

LeanidShutau commented Oct 2, 2018 •

edited

Loading

pavel06081991 Oct 2, 2018 •

edited

Loading

pavel06081991 commented Oct 2, 2018 •

edited

Loading