From b0568b14375112cc8ff6734ec491f1930ad7be46 Mon Sep 17 00:00:00 2001 From: Filipe Dobreira Date: Wed, 13 Mar 2024 15:41:36 +0000 Subject: [PATCH 1/4] fix: pre-process comment text on backend + frontend --- .../src/controllers/commentsController.ts | 20 +++++++++---------- .../src/models/CommentModel/CommentModel.ts | 9 +++++++-- .../services/CommentService/CommentService.ts | 4 ++-- .../comments/components/CommentDetail.tsx | 15 +++++++++++--- 4 files changed, 30 insertions(+), 18 deletions(-) diff --git a/packages/backend/src/controllers/commentsController.ts b/packages/backend/src/controllers/commentsController.ts index a19099fab232..bdb0f7ed73b9 100644 --- a/packages/backend/src/controllers/commentsController.ts +++ b/packages/backend/src/controllers/commentsController.ts @@ -46,17 +46,15 @@ export class CommentsController extends BaseController { @Body() body: Pick, ): Promise { - const commentId = await this.services - .getCommentService() - .createComment( - req.user!, - dashboardUuid, - dashboardTileUuid, - body.text, - body.textHtml, - body.replyTo ?? null, - body.mentions, - ); + const commentId = await this.services.getCommentService().createComment( + req.user!, + dashboardUuid, + dashboardTileUuid, + body.text, + body.textHtml, // not yet sanitized + body.replyTo ?? null, + body.mentions, + ); this.setStatus(200); return { status: 'ok', diff --git a/packages/backend/src/models/CommentModel/CommentModel.ts b/packages/backend/src/models/CommentModel/CommentModel.ts index b8f5203ff5c6..02320b400db3 100644 --- a/packages/backend/src/models/CommentModel/CommentModel.ts +++ b/packages/backend/src/models/CommentModel/CommentModel.ts @@ -1,4 +1,9 @@ -import { Comment, LightdashUser, NotFoundError } from '@lightdash/common'; +import { + Comment, + LightdashUser, + NotFoundError, + sanitizeHtml, +} from '@lightdash/common'; import { Knex } from 'knex'; import { DashboardTileCommentsTableName, @@ -159,7 +164,7 @@ export class CommentModel { const [comment] = await this.database(DashboardTileCommentsTableName) .insert({ text, - text_html: textHtml, + text_html: sanitizeHtml(textHtml), dashboard_tile_uuid: dashboardTileUuid, reply_to: replyTo ?? null, user_uuid: user.userUuid, diff --git a/packages/backend/src/services/CommentService/CommentService.ts b/packages/backend/src/services/CommentService/CommentService.ts index fe0d2ce9dc78..eb3385edbd95 100644 --- a/packages/backend/src/services/CommentService/CommentService.ts +++ b/packages/backend/src/services/CommentService/CommentService.ts @@ -134,7 +134,7 @@ export class CommentService { dashboardUuid: string, dashboardTileUuid: string, text: string, - textHtml: string, + unsafeTextHtml: string, replyTo: string | null, mentions: string[], ): Promise { @@ -172,7 +172,7 @@ export class CommentService { dashboardUuid, dashboardTileUuid, text, - textHtml, + unsafeTextHtml, replyTo, user, mentions, diff --git a/packages/frontend/src/features/comments/components/CommentDetail.tsx b/packages/frontend/src/features/comments/components/CommentDetail.tsx index bc7a7c1e22d4..d5dff2579256 100644 --- a/packages/frontend/src/features/comments/components/CommentDetail.tsx +++ b/packages/frontend/src/features/comments/components/CommentDetail.tsx @@ -1,4 +1,4 @@ -import { type Comment } from '@lightdash/common'; +import { sanitizeHtml, type Comment } from '@lightdash/common'; import { ActionIcon, Avatar, @@ -11,7 +11,7 @@ import { } from '@mantine/core'; import { useHover } from '@mantine/hooks'; import { IconDotsVertical, IconMessage, IconTrash } from '@tabler/icons-react'; -import { type FC } from 'react'; +import { useMemo, type FC } from 'react'; import MantineIcon from '../../../components/common/MantineIcon'; import { getNameInitials } from '../utils'; import { CommentTimestamp } from './CommentTimestamp'; @@ -33,6 +33,15 @@ export const CommentDetail: FC = ({ }) => { const { ref, hovered } = useHover(); + /** + * Content should already be sanitized from the API, but as an extra + * precaution we also sanitize it before rendering. + */ + const sanitizedCommentTextHtml = useMemo( + () => sanitizeHtml(comment.textHtml), + [comment.textHtml], + ); + return ( @@ -97,7 +106,7 @@ export const CommentDetail: FC = ({ Date: Wed, 13 Mar 2024 15:53:03 +0000 Subject: [PATCH 2/4] fix: adjust sanitize method to allow mentions + improve tests --- .../src/models/CommentModel/CommentModel.ts | 10 ++++++++-- .../common/src/utils/sanitizeHtml.test.ts | 20 +++++++++++++++---- packages/common/src/utils/sanitizeHtml.ts | 10 +++++++++- 3 files changed, 33 insertions(+), 7 deletions(-) diff --git a/packages/backend/src/models/CommentModel/CommentModel.ts b/packages/backend/src/models/CommentModel/CommentModel.ts index 02320b400db3..6eb80e8def24 100644 --- a/packages/backend/src/models/CommentModel/CommentModel.ts +++ b/packages/backend/src/models/CommentModel/CommentModel.ts @@ -151,7 +151,7 @@ export class CommentModel { dashboardUuid: string, dashboardTileUuid: string, text: string, - textHtml: string, + unsafeTextHtml: string, replyTo: string | null, user: LightdashUser, mentions: string[], @@ -161,10 +161,16 @@ export class CommentModel { dashboardUuid, dashboardTileUuid, ); + console.log( + 'INPUT HTML', + unsafeTextHtml, + 'OUTPUT', + sanitizeHtml(unsafeTextHtml), + ); const [comment] = await this.database(DashboardTileCommentsTableName) .insert({ text, - text_html: sanitizeHtml(textHtml), + text_html: sanitizeHtml(unsafeTextHtml), dashboard_tile_uuid: dashboardTileUuid, reply_to: replyTo ?? null, user_uuid: user.userUuid, diff --git a/packages/common/src/utils/sanitizeHtml.test.ts b/packages/common/src/utils/sanitizeHtml.test.ts index d29bdc66cc92..368f0a4814ea 100644 --- a/packages/common/src/utils/sanitizeHtml.test.ts +++ b/packages/common/src/utils/sanitizeHtml.test.ts @@ -5,30 +5,42 @@ describe('sanitizeHtml', () => { expect(sanitizeHtml('')).toEqual(''); }); - test('no html tags', () => { + test('no html tags in input', () => { expect(sanitizeHtml('Hello this is just some text')).toEqual( 'Hello this is just some text', ); }); - test('script tag (discard)', () => { + test('script tag is discarded', () => { expect(sanitizeHtml('')).toEqual( '', ); }); - test('script tag with neighboring text', () => { + test('script tag with neighboring text is discarded', () => { expect( sanitizeHtml(' Some text'), ).toEqual(' Some text'); }); - test('valid tag', () => { + test('valid tag is preserved', () => { expect( sanitizeHtml('Lightdash'), ).toEqual('Lightdash'); }); + test('style tag in span is preserved', () => { + expect( + sanitizeHtml('@Foo'), + ).toEqual('@Foo'); + }); + + test('style tag in paragraph is discarded', () => { + expect( + sanitizeHtml('

@Foo

'), + ).toEqual('

@Foo

'); + }); + test('valid tag with surrounding + inner text', () => { expect( sanitizeHtml('Here is some text

And a paragraph.


'), diff --git a/packages/common/src/utils/sanitizeHtml.ts b/packages/common/src/utils/sanitizeHtml.ts index 14ae12cccad2..ef505846df95 100644 --- a/packages/common/src/utils/sanitizeHtml.ts +++ b/packages/common/src/utils/sanitizeHtml.ts @@ -4,7 +4,15 @@ import sanitize from 'sanitize-html'; * If you want to modify sanitization settings, be sure to merge them * with the sane defaults pre-included with sanitize-html. */ -const HTML_SANITIZE_OPTIONS: sanitize.IOptions = sanitize.defaults; +const HTML_SANITIZE_OPTIONS: sanitize.IOptions = { + ...sanitize.defaults, + + allowedAttributes: { + ...sanitize.defaults.allowedAttributes, + // Allow @mentions to be styled differently: + span: [...(sanitize.defaults.allowedAttributes.span ?? []), 'style'], + }, +}; export const sanitizeHtml = (input: string): string => sanitize(input, HTML_SANITIZE_OPTIONS); From 16426da8bc7322d8d4f37c995a591e23f22cafd9 Mon Sep 17 00:00:00 2001 From: Filipe Dobreira Date: Wed, 13 Mar 2024 15:55:54 +0000 Subject: [PATCH 3/4] cleanup log statement --- packages/backend/src/models/CommentModel/CommentModel.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/backend/src/models/CommentModel/CommentModel.ts b/packages/backend/src/models/CommentModel/CommentModel.ts index 6eb80e8def24..1855b443302d 100644 --- a/packages/backend/src/models/CommentModel/CommentModel.ts +++ b/packages/backend/src/models/CommentModel/CommentModel.ts @@ -161,12 +161,6 @@ export class CommentModel { dashboardUuid, dashboardTileUuid, ); - console.log( - 'INPUT HTML', - unsafeTextHtml, - 'OUTPUT', - sanitizeHtml(unsafeTextHtml), - ); const [comment] = await this.database(DashboardTileCommentsTableName) .insert({ text, From 5e9a1b0da09ee4eaa4a6fe91a4bd573556b13320 Mon Sep 17 00:00:00 2001 From: Filipe Dobreira Date: Wed, 13 Mar 2024 16:26:38 +0000 Subject: [PATCH 4/4] add sanity test for double sanitization + malformed tags --- packages/common/src/utils/sanitizeHtml.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/common/src/utils/sanitizeHtml.test.ts b/packages/common/src/utils/sanitizeHtml.test.ts index 368f0a4814ea..4490cd413029 100644 --- a/packages/common/src/utils/sanitizeHtml.test.ts +++ b/packages/common/src/utils/sanitizeHtml.test.ts @@ -46,4 +46,20 @@ describe('sanitizeHtml', () => { sanitizeHtml('Here is some text

And a paragraph.


'), ).toEqual('Here is some text

And a paragraph.


'); }); + + test('double sanitization (with invalid tags)', () => { + expect( + sanitizeHtml( + sanitizeHtml( + '

@Foo

', + ), + ), + ).toEqual('

@Foo

'); + }); + + test('malformed tag', () => { + expect(sanitizeHtml('@Foo')).toEqual( + '@Foo', + ); + }); });