Skip to content

Enabling Markdown for GristDocTour #1653

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Enabling Markdown for GristDocTour #1653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf25379
Enabling Markdown for GristDocTour
ogui11aume Jun 3, 2025
26d9687
Tests for Markdown in DocTour
ogui11aume Aug 1, 2025
de030e1
fix(test): Change the parent html element and change assert to true
ogui11aume Aug 6, 2025
7223e3c
Refactor DOM_FRAGMENT and ALLOWED_TAGS
ogui11aume Aug 6, 2025
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
22 changes: 16 additions & 6 deletions app/client/ui/DocTour.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import {IconList, IconName} from 'app/client/ui2018/IconList';
import {DocData} from 'app/common/DocData';
import {dom} from 'grainjs';
import sortBy = require('lodash/sortBy');
import {marked} from "marked";
import {renderer} from 'app/client/ui/DocTutorialRenderer';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import {renderer} from 'app/client/ui/DocTutorialRenderer';
import {renderer} from 'app/client/ui/DocTourRenderer';

Copy link
Contributor

Choose a reason for hiding this comment

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

DocTourRenderer isn't being imported here.

import {sanitizeTourHTML} from "app/client/ui/sanitizeHTML";

const t = makeT('DocTour');

Expand Down Expand Up @@ -44,17 +47,22 @@ async function makeDocTour(docData: DocData, docComm: DocComm): Promise<IOnBoard
return String(tableData.getValue(rowId, colId) || "");
}
const title = getValue("Title");
let body: HTMLElement | string = getValue("Body");
const bodyValue = getValue("Body");

if (!title && !(bodyValue.trim()) ) {
return null;
}
//const bodyHtmlContent
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//const bodyHtmlContent

const element = sanitizeTourHTML(marked.parse(bodyValue, {
async: false, renderer
}));

const linkText = getValue("Link_Text");
const linkUrl = getValue("Link_URL");
const linkIcon = getValue("Link_Icon") as IconName;
const locationValue = getValue("Location");
let placement = getValue("Placement");

if (!(title || body)) {
return null;
}

const urlState = sameDocumentUrlState(locationValue);
if (isNarrowScreen() || !placements.includes(placement as Placement)) {
placement = "auto";
Expand All @@ -67,10 +75,12 @@ async function makeDocTour(docData: DocData, docComm: DocComm): Promise<IOnBoard
validLinkUrl = false;
}

let body: HTMLElement = dom('div', element);

if (validLinkUrl && linkText) {
body = dom(
'div',
dom('p', body),
body,
dom('p',
cssButtons(cssLinkBtn(
IconList.includes(linkIcon) ? cssLinkIcon(linkIcon) : null,
Expand Down
8 changes: 8 additions & 0 deletions app/client/ui/DocTourRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import {constructUrl} from 'app/client/models/gristUrlState';
import {gristIconLink} from 'app/client/ui2018/links';
import {marked} from 'marked';

export const renderer = new marked.Renderer();

renderer.link = ({href, text}) =>
Copy link
Contributor

@berhalak berhalak Jul 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think that the default marked.Renderer allows arbitrary html, which potentially allows some phishing attacks. For example with <div style="position: fixed; color: red; inset: 0">Boo</div> you can cover whole Grist layout and provide your own.

What do you think about reusing the renderer from app/client/ui/MarkdownCellRenderer.ts? It clears out any html content.

Copy link
Author

Choose a reason for hiding this comment

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

I used ALLOWED_TAGS with marked.Renderer in app/client/ui/sanitizeHTML.ts to limit abuse and only allow those HTML elements that have been tested (+ table).

  • Agreed, this does not yet avoid the problem you described ( covering div ). To avoid the problem in your example, I would also need to limit the usage of style at least in div, should I go that far ?
  • The problem with app/client/ui/MarkdownCellRenderer.ts is that, according to comment, it will not permit images or other HTML elements. So I didn't go that way.

    grist-core/app/client/ui/MarkdownCellRenderer.ts

    Line 59 in 129a053

    * This does not support HTML or images in the markdown value, and renders those as text.
    :

This does not support HTML or images in the markdown value, and renders those as text.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ogui11aume, what if we disallowed HTML for the time being?

renderer.html = ({raw}) => escape(raw);

Would there be an immediate need for tours to support arbitrary HTML?

Having a robust and shared sanitization approach across different UI components like tutorials, tours, etc. seems desirable, but could perhaps be reconciled later. (Tutorials in particular should probably also disallow HTML outside of the specific iframe that's permitted, but that's unrelated to your proposed changes.)

gristIconLink(constructUrl(href), text).outerHTML;
14 changes: 14 additions & 0 deletions app/client/ui/sanitizeHTML.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,19 @@ export function sanitizeHTMLIntoDOM(source: string | Node): DocumentFragment {
}
}

export function sanitizeTourHTML(source: string | Node): DocumentFragment {
return tourPurifier.sanitize(source, {
RETURN_DOM_FRAGMENT: true,
ALLOWED_TAGS: [
'p', 'div', 'span', 'h1', 'h2', 'h3', 'h4', 'table', 'tr', 'td',
'strong', 'em', 'bold', 'code', 'pre', 'blockquote', 'ul', 'ol', 'li', 'del',
'br', 'img', 'iframe', 'a'
],
ADD_ATTR: ['allowFullscreen']
});

}

export function sanitizeTutorialHTML(source: string | Node): string {
return tutorialPurifier.sanitize(source, {
ADD_TAGS: ['iframe'],
Expand All @@ -35,6 +48,7 @@ export function sanitizeTutorialHTML(source: string | Node): string {

const defaultPurifier = createDOMPurifier();
const tutorialPurifier = createDOMPurifier();
const tourPurifier = createDOMPurifier();

// If we are executed in a browser, we can add hooks to the purifiers to customize their behavior.
// But sometimes this code is included in tests, where `window` is not defined.
Expand Down
Binary file modified test/fixtures/docs/doctour.grist
View file
Open in desktop
Binary file not shown.
125 changes: 119 additions & 6 deletions test/nbrowser/DocTour.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,19 @@ describe('DocTour', function () {
const docTour = await driver.executeScript('return window._gristDocTour()');
assert.deepEqual(docTour, [
{
body: 'General Kenobi!',
body: '<div>' +
'<p>General Kenobi!</p>\n' +
'</div>',
placement: 'bottom',
selector: '.active_cursor',
showHasModal: false,
title: 'Hello there!',
urlState: {colRef: 2, rowId: 2, sectionId: 1}
},
{
body: 'no title',
body: '<div>' +
'<p>no title</p>\n' +
'</div>',
placement: 'auto',
selector: '.active_cursor',
showHasModal: true,
Expand All @@ -64,7 +68,7 @@ describe('DocTour', function () {
},
{
body: '<div>' +
'<p></p>' +
'<div></div>' +
'<p><div class="_grainXXX_">' +
'<a href="https://www.getgrist.com/" target="_blank" class="_grainXXX_ _grainXXX_">' +
'<div class="_grainXXX_ _grainXXX_" style="mask-image: var(--icon-Page);"></div>' +
Expand All @@ -79,8 +83,8 @@ describe('DocTour', function () {
urlState: {colRef: 4, rowId: 4, sectionId: 1}
},
{
body: '<div>' +
'<p>Good riddance</p>' +
body: '<div><div>' +
'<p>Good riddance</p>\n</div>' +
'<p><div class="_grainXXX_">' +
'<a href="https://xkcd.com/" target="_blank" class="_grainXXX_ _grainXXX_">' +
'Test link here' +
Expand All @@ -92,6 +96,113 @@ describe('DocTour', function () {
showHasModal: true,
title: 'Bye',
urlState: null,
},
{
body: '<div><p>' +
'<strong>bold</strong>' +
' <em>italicized</em>' +
' <code>code</code>' +
' <del>strikethrough</del>' +
'</p>\n' +
'</div>',
placement: 'auto',
selector: '.active_cursor',
showHasModal: true,
title: 'Markdown formating',
urlState: null,
},
{
body: '<div><p></p>' +
'<div class="doc-tutorial-popup-thumbnail">\n' +
' <img title="" src="https://example.com/image.jpg">\n' +
' <div class="doc-tutorial-popup-thumbnail-icon-wrapper">\n' +
' <div class="doc-tutorial-popup-thumbnail-icon"></div>\n' +
' </div>\n</div><p></p>\n' +
'</div>',
placement: 'auto',
selector: '.active_cursor',
showHasModal: false,
title: 'Markdown image',
urlState: {
"colRef": 8,
"rowId": 8,
"sectionId": 1
},
},
{
body: '<div>' +
'<h1>H1</h1>\n<h2>H2</h2>\n<h3>H3</h3>\n' +
'</div>',
placement: 'auto',
selector: '.active_cursor',
showHasModal: true,
title: 'Markdown headings',
urlState: null,
},
{
body: '<div>' +
'<ol>\n<li>First item</li>\n<li>Second item</li>\n<li>Third item</li>\n</ol>\n' +
'</div>',
placement: 'auto',
selector: '.active_cursor',
showHasModal: false,
title: 'Markdown ordered list',
urlState: {
"colRef": 10,
"rowId": 10,
"sectionId": 1
},
},
{
body: '<div>' +
'<ul>\n<li>First item</li>\n<li>Second item</li>\n<li>Third item</li>\n</ul>\n' +
'</div>',
placement: 'auto',
selector: '.active_cursor',
showHasModal: true,
title: 'Markdown unordered list',
urlState: null,
},
{
body: '<div>' +
'<blockquote>\n<p>blockquote</p>\n</blockquote>\n' +
'</div>',
placement: 'auto',
selector: '.active_cursor',
showHasModal: false,
title: 'Markdown quote',
urlState: {
"colRef": 12,
"rowId": 12,
"sectionId": 1
},
},
{
body: '<div>' +
'<pre><code>{\n ' +
'"firstName": "John",\n "lastName": "Smith",\n "age": 25\n' +
'}\n' +
'</code></pre>\n' +
'</div>',
placement: 'auto',
selector: '.active_cursor',
showHasModal: true,
title: 'Markdown code block',
urlState: null,
},
{
body: '<div>' +
'<p><a href="https://www.example.com">title</a></p>\n' +
'</div>',
placement: 'auto',
selector: '.active_cursor',
showHasModal: false,
title: 'Markdown link',
urlState: {
"colRef": 14,
"rowId": 14,
"sectionId": 1
},
}
]);
});
Expand Down Expand Up @@ -154,7 +265,9 @@ describe('DocTour', function () {

async function checkDocTourPresent() {
// Check the expected message.
assert.match(await driver.findWait('.test-onboarding-popup', 500).getText(), /General Kenobi/);
assert.isTrue(await driver.findWait('.test-onboarding-popup', 500)
.findContent('div', 'General Kenobi!').isPresent());

// Check that there is only one popup, and no errors.
assert.lengthOf(await driver.findAll('.test-onboarding-popup'), 1);
await gu.checkForErrors();
Expand Down
Loading