Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[I18n] Compile html in angular directive #23684

Closed
wants to merge 10 commits into from
Closed

[I18n] Compile html in angular directive #23684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8d14ffc
add ability to compile child directives
Jul 5, 2018
6486b93
Add typings and update tests
LeanidShutau Sep 28, 2018
0a8490e
Merge remote-tracking branch 'upstream/master' into feature/compile-h…
LeanidShutau Oct 8, 2018
71115a2
Merge remote-tracking branch 'upstream/master' into feature/compile-h…
LeanidShutau Oct 10, 2018
81b29c0
Sanitize safe values
LeanidShutau Oct 16, 2018
c551b36
Merge remote-tracking branch 'upstream/master' into feature/compile-h…
LeanidShutau Oct 16, 2018
ad5ba8c
Merge remote-tracking branch 'upstream/master' into feature/compile-h…
LeanidShutau Oct 17, 2018
de46c9b
Escape html tags in defaultMessage
LeanidShutau Oct 17, 2018
61c4f2b
Fix tests
LeanidShutau Oct 17, 2018
2b441cc
Merge remote-tracking branch 'upstream/master' into feature/compile-h…
LeanidShutau Oct 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions packages/kbn-i18n/src/angular/__snapshots__/directive.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,47 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`i18nDirective inserts correct translation html content 1`] = `
<div
class="ng-scope ng-isolate-scope"
i18n-default-message="Default message"
i18n-id="id"
>
Default message
</div>
`;

exports[`i18nDirective inserts correct translation html content with values 1`] = `"default-message word"`;

exports[`i18nDirective inserts correct translation html content with values 2`] = `"default-message anotherWord"`;

exports[`i18nDirective sanitizes defaultMessage 1`] = `
<div
class="ng-scope ng-isolate-scope"
i18n-default-message="Dangerous default message, {unsafe_value}"
i18n-id="id"
i18n-values="{ unsafe_value: '<div i18n-id=\\"id2\\" i18n-default-message=\\"<script></script>inner message\\" />' }"
>
Dangerous default message,
<div
class="ng-scope ng-isolate-scope"
i18n-default-message="<script></script>inner message"
i18n-id="id2"
>
&lt;script&gt;&lt;/script&gt;inner message
</div>
</div>
`;

exports[`i18nDirective sanitizes safe value 1`] = `
<div
class="ng-scope ng-isolate-scope"
i18n-default-message="Default message, {value}"
i18n-id="id"
i18n-values="{ value: '<div ng-click=\\"dangerousAction()\\"></div>' }"
>
Default message,
<div
class="ng-scope"
/>
</div>
`;
40 changes: 34 additions & 6 deletions packages/kbn-i18n/src/angular/directive.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,13 @@

import angular from 'angular';
import 'angular-mocks';
import 'angular-sanitize';

import { i18nDirective } from './directive';
import { I18nProvider } from './provider';

angular
.module('app', [])
.module('app', ['ngSanitize'])
.provider('i18n', I18nProvider)
.directive('i18nId', i18nDirective);

Expand All @@ -44,20 +45,47 @@ describe('i18nDirective', () => {
);

test('inserts correct translation html content', () => {
const id = 'id';
const defaultMessage = 'default-message';
const element = angular.element(
`<div
i18n-id="id"
i18n-default-message="Default message"
/>`
);

compile(element)(scope);
scope.$digest();

expect(element[0]).toMatchSnapshot();
});

test('sanitizes defaultMessage', () => {
const element = angular.element(
`<div
i18n-id="${id}"
i18n-default-message="${defaultMessage}"
i18n-id="id"
i18n-default-message="Dangerous default message, {unsafe_value}"
i18n-values="{ unsafe_value: '<div i18n-id=&quot;id2&quot; i18n-default-message=&quot;<script></script>inner message&quot; />' }"
/>`
);

compile(element)(scope);
scope.$digest();

expect(element[0]).toMatchSnapshot();
});

test('sanitizes safe value', () => {
const element = angular.element(
`<div
i18n-id="id"
i18n-default-message="Default message, {value}"
i18n-values="{ value: '<div ng-click=&quot;dangerousAction()&quot;></div>' }"
/>`
);

compile(element)(scope);
scope.$digest();

expect(element.html()).toEqual(defaultMessage);
expect(element[0]).toMatchSnapshot();
});

test('inserts correct translation html content with values', () => {
Expand Down
37 changes: 30 additions & 7 deletions packages/kbn-i18n/src/angular/directive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { IDirective, IRootElementService, IScope } from 'angular';
import { ICompileService, IDirective, IRootElementService, IScope } from 'angular';

import { I18nServiceType } from './provider';

Expand All @@ -27,7 +27,11 @@ interface I18nScope extends IScope {
id: string;
}

export function i18nDirective(i18n: I18nServiceType): IDirective<I18nScope> {
export function i18nDirective(
i18n: I18nServiceType,
$compile: ICompileService,
$sanitize: (html: string) => string
): IDirective<I18nScope> {
return {
restrict: 'A',
scope: {
Expand All @@ -38,20 +42,39 @@ export function i18nDirective(i18n: I18nServiceType): IDirective<I18nScope> {
link($scope, $element) {
if ($scope.values) {
$scope.$watchCollection('values', () => {
setHtmlContent($element, $scope, i18n);
setHtmlContent($element, $scope, $sanitize, i18n);
$compile($element.contents() as any)($scope.$parent);
Copy link
Contributor

@pavel06081991 pavel06081991 Oct 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. we have agreed not to use tags in defaultMessage.
  2. 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

  1. Simple default message + simple values, in this case we should just use element.text(i18n.translate(...)) - text will escape everything for us.
  2. 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
  3. 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

});
} else {
setHtmlContent($element, $scope, i18n);
setHtmlContent($element, $scope, $sanitize, i18n);
}
},
};
}

function setHtmlContent($element: IRootElementService, $scope: I18nScope, i18n: I18nServiceType) {
function setHtmlContent(
$element: IRootElementService,
$scope: I18nScope,
$sanitize: (html: string) => string,
i18n: I18nServiceType
) {
const values = Object.entries($scope.values || {}).reduce(
(result, [key, value]) => {
if (key.startsWith('unsafe_')) {
result[key] = value;
} else {
result[key] = $sanitize(value);
}

return result;
},
{} as Record<string, any>
);

$element.html(
i18n($scope.id, {
values: $scope.values,
defaultMessage: $scope.defaultMessage,
values,
defaultMessage: ($scope.defaultMessage || '').replace(/\</g, '&lt;').replace(/\>/g, '&gt;'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})
);
}