From a7ec8088e475c0ce811e96fdc846172568cbfd84 Mon Sep 17 00:00:00 2001 From: Alex Tugarev Date: Fri, 6 Mar 2020 13:55:13 +0000 Subject: [PATCH] [notifications] disallow arbitrary html in message content this way we can be sure that no scripts can be executed. Signed-off-by: Alex Tugarev --- .../src/browser/messages-frontend-module.ts | 2 + .../notification-content-renderer.spec.ts | 75 +++++++++++++++++++ .../notification-content-renderer.ts} | 24 +++--- .../src/browser/notifications-manager.ts | 14 ++-- 4 files changed, 96 insertions(+), 19 deletions(-) create mode 100644 packages/messages/src/browser/notification-content-renderer.spec.ts rename packages/messages/src/{package.spec.ts => browser/notification-content-renderer.ts} (60%) diff --git a/packages/messages/src/browser/messages-frontend-module.ts b/packages/messages/src/browser/messages-frontend-module.ts index aed1073a60a66..7ce6a521f190d 100644 --- a/packages/messages/src/browser/messages-frontend-module.ts +++ b/packages/messages/src/browser/messages-frontend-module.ts @@ -25,8 +25,10 @@ import { NotificationsContribution, NotificationsKeybindingContext } from './not import { FrontendApplicationContribution, KeybindingContribution, KeybindingContext } from '@theia/core/lib/browser'; import { CommandContribution } from '@theia/core'; import { ColorContribution } from '@theia/core/lib/browser/color-application-contribution'; +import { NotificationContentRenderer } from './notification-content-renderer'; export default new ContainerModule((bind, unbind, isBound, rebind) => { + bind(NotificationContentRenderer).toSelf().inSingletonScope(); bind(NotificationsRenderer).toSelf().inSingletonScope(); bind(NotificationsContribution).toSelf().inSingletonScope(); bind(FrontendApplicationContribution).toService(NotificationsContribution); diff --git a/packages/messages/src/browser/notification-content-renderer.spec.ts b/packages/messages/src/browser/notification-content-renderer.spec.ts new file mode 100644 index 0000000000000..936576116398f --- /dev/null +++ b/packages/messages/src/browser/notification-content-renderer.spec.ts @@ -0,0 +1,75 @@ +/******************************************************************************** + * Copyright (C) 2020 TypeFox and others. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary + * Licenses when the conditions for such availability set forth in the Eclipse + * Public License v. 2.0 are satisfied: GNU General Public License, version 2 + * with the GNU Classpath Exception which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + ********************************************************************************/ + +import { expect } from 'chai'; +import { NotificationContentRenderer } from './notification-content-renderer'; + +/* eslint-disable no-unused-expressions */ + +describe('notification-content-renderer', () => { + + const contentRnderer = new NotificationContentRenderer(); + + it('should remove new lines', () => { + expectRenderedContent('foo\nbar', 'foo bar'); + expectRenderedContent('foo\n\n\nbar', 'foo bar'); + }); + + it('should render links', () => { + expectRenderedContent( + 'Link to [theia](https://github.com/eclipse-theia/theia)!', + 'Link to theia!' + ); + expectRenderedContent( + 'Link to [theia](https://github.com/eclipse-theia/theia "title on hover")!', + 'Link to theia!' + ); + expectRenderedContent( + 'Click [here](command:my-command-id) to open stuff!', + 'Click here to open stuff!' + ); + expectRenderedContent( + 'Click [here](javascript:window.alert();) to open stuff!', + 'Click [here](javascript:window.alert();) to open stuff!' + ); + }); + + it('should render markdown', () => { + expectRenderedContent( + '*italic*', + 'italic' + ); + expectRenderedContent( + '**bold**', + 'bold' + ); + }); + + it('should not render html', () => { + expectRenderedContent( + '', + '<script>document.getElementById("demo").innerHTML = "Hello JavaScript!";</script>' + ); + expectRenderedContent( + 'foobar', + '<a href="javascript:window.alert();">foobar</a>' + ); + }); + + const expectRenderedContent = (input: string, output: string) => + expect(contentRnderer.renderMessage(input)).to.be.equal(output); + +}); diff --git a/packages/messages/src/package.spec.ts b/packages/messages/src/browser/notification-content-renderer.ts similarity index 60% rename from packages/messages/src/package.spec.ts rename to packages/messages/src/browser/notification-content-renderer.ts index b15ce7e665ae5..2c99b2cc72a28 100644 --- a/packages/messages/src/package.spec.ts +++ b/packages/messages/src/browser/notification-content-renderer.ts @@ -1,5 +1,5 @@ /******************************************************************************** - * Copyright (C) 2017 TypeFox and others. + * Copyright (C) 2020 TypeFox and others. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v. 2.0 which is available at @@ -14,16 +14,18 @@ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 ********************************************************************************/ -/* note: this bogus test file is required so that - we are able to run mocha unit tests on this - package, without having any actual unit tests in it. - This way a coverage report will be generated, - showing 0% coverage, instead of no report. - This file can be removed once we have real unit - tests in place. */ +import * as markdownit from 'markdown-it'; +import { injectable } from 'inversify'; -describe('messages package', () => { +@injectable() +export class NotificationContentRenderer { - it('support code coverage statistics', () => true); + protected readonly mdEngine = markdownit({ html: false }); -}); + renderMessage(content: string): string { + // in alignment with vscode, new lines aren't supported + const contentWithoutNewlines = content.replace(/((\r)?\n)+/gm, ' '); + + return this.mdEngine.renderInline(contentWithoutNewlines); + } +} diff --git a/packages/messages/src/browser/notifications-manager.ts b/packages/messages/src/browser/notifications-manager.ts index 421c8c5c5f2c3..2c0f7b00fd752 100644 --- a/packages/messages/src/browser/notifications-manager.ts +++ b/packages/messages/src/browser/notifications-manager.ts @@ -20,12 +20,12 @@ import { deepClone } from '@theia/core/lib/common/objects'; import { Emitter } from '@theia/core'; import { Deferred } from '@theia/core/lib/common/promise-util'; import { Md5 } from 'ts-md5'; -import * as markdownit from 'markdown-it'; import throttle = require('lodash.throttle'); import { NotificationPreferences } from './notification-preferences'; import { ContextKeyService, ContextKey } from '@theia/core/lib/browser/context-key-service'; import { OpenerService } from '@theia/core/lib/browser'; import URI from '@theia/core/lib/common/uri'; +import { NotificationContentRenderer } from './notification-content-renderer'; export interface NotificationUpdateEvent { readonly notifications: Notification[]; @@ -60,6 +60,9 @@ export class NotificationManager extends MessageClient { @inject(OpenerService) protected readonly openerService: OpenerService; + @inject(NotificationContentRenderer) + protected readonly contentRenderer: NotificationContentRenderer; + protected readonly onUpdatedEmitter = new Emitter(); readonly onUpdated = this.onUpdatedEmitter.event; protected readonly fireUpdatedEvent = throttle(() => { @@ -164,7 +167,7 @@ export class NotificationManager extends MessageClient { let notification = this.notifications.get(messageId); if (!notification) { - const message = this.renderMessage(plainMessage.text); + const message = this.contentRenderer.renderMessage(plainMessage.text); const type = this.toNotificationType(plainMessage.type); const actions = Array.from(new Set(plainMessage.actions)); const source = plainMessage.source; @@ -208,11 +211,6 @@ export class NotificationManager extends MessageClient { } return plainMessage.options && plainMessage.options.timeout || this.preferences['notification.timeout']; } - protected readonly mdEngine = markdownit({ html: true }); - protected renderMessage(content: string): string { - const contentWithoutNewlines = content.replace(/(\r)?\n/gm, ' '); - return this.mdEngine.renderInline(contentWithoutNewlines); - } protected isExpandable(message: string, source: string | undefined, actions: string[]): boolean { if (!actions.length && source) { return true; @@ -238,7 +236,7 @@ export class NotificationManager extends MessageClient { async showProgress(messageId: string, plainMessage: ProgressMessage, cancellationToken: CancellationToken): Promise { let notification = this.notifications.get(messageId); if (!notification) { - const message = this.renderMessage(plainMessage.text); + const message = this.contentRenderer.renderMessage(plainMessage.text); const type = this.toNotificationType(plainMessage.type); const actions = Array.from(new Set(plainMessage.actions)); const source = plainMessage.source;