Merge pull request from GHSA-4m77-cmpx-vjc4

* Fix sanitization and escaping for MD headings in ToC * Add a unit test for `getHeadingId` * Make `sanitizer` argument optional in getHeadingId, integrity update (cherry picked from commit e1b3aab)
jupyterlab · Jan 19, 2024 · dda0033 · dda0033
1 parent 0708330
commit dda0033
Show file tree

Hide file tree

Showing 11 changed files with 85 additions and 26 deletions.
diff --git a/packages/markdownviewer-extension/src/index.ts b/packages/markdownviewer-extension/src/index.ts
@@ -10,7 +10,7 @@ import {
   JupyterFrontEnd,
   JupyterFrontEndPlugin
 } from '@jupyterlab/application';
-import { WidgetTracker } from '@jupyterlab/apputils';
+import { ISanitizer, WidgetTracker } from '@jupyterlab/apputils';
 import { PathExt } from '@jupyterlab/coreutils';
 import {
   IMarkdownViewerTracker,
@@ -20,6 +20,7 @@ import {
   MarkdownViewerTableOfContentsFactory
 } from '@jupyterlab/markdownviewer';
 import {
+  IRenderMime,
   IRenderMimeRegistry,
   markdownRendererFactory
 } from '@jupyterlab/rendermime';
@@ -49,7 +50,12 @@ const plugin: JupyterFrontEndPlugin<IMarkdownViewerTracker> = {
   description: 'Adds markdown file viewer and provides its tracker.',
   provides: IMarkdownViewerTracker,
   requires: [IRenderMimeRegistry, ITranslator],
-  optional: [ILayoutRestorer, ISettingRegistry, ITableOfContentsRegistry],
+  optional: [
+    ILayoutRestorer,
+    ISettingRegistry,
+    ITableOfContentsRegistry,
+    ISanitizer
+  ],
   autoStart: true
 };
 
@@ -62,7 +68,8 @@ function activate(
   translator: ITranslator,
   restorer: ILayoutRestorer | null,
   settingRegistry: ISettingRegistry | null,
-  tocRegistry: ITableOfContentsRegistry | null
+  tocRegistry: ITableOfContentsRegistry | null,
+  sanitizer: IRenderMime.ISanitizer | null
 ): IMarkdownViewerTracker {
   const trans = translator.load('jupyterlab');
   const { commands, docRegistry } = app;
@@ -182,7 +189,8 @@ function activate(
     tocRegistry.add(
       new MarkdownViewerTableOfContentsFactory(
         tracker,
-        rendermime.markdownParser
+        rendermime.markdownParser,
+        sanitizer ?? rendermime.sanitizer
       )
     );
   }

diff --git a/packages/markdownviewer/src/toc.ts b/packages/markdownviewer/src/toc.ts
@@ -2,7 +2,7 @@
 // Distributed under the terms of the Modified BSD License.
 
 import { IWidgetTracker } from '@jupyterlab/apputils';
-import { IMarkdownParser } from '@jupyterlab/rendermime';
+import { IMarkdownParser, IRenderMime } from '@jupyterlab/rendermime';
 import {
   TableOfContents,
   TableOfContentsFactory,
@@ -96,7 +96,8 @@ export class MarkdownViewerTableOfContentsFactory extends TableOfContentsFactory
    */
   constructor(
     tracker: IWidgetTracker<MarkdownDocument>,
-    protected parser: IMarkdownParser | null
+    protected parser: IMarkdownParser | null,
+    protected sanitizer: IRenderMime.ISanitizer
   ) {
     super(tracker);
   }
@@ -165,13 +166,14 @@ export class MarkdownViewerTableOfContentsFactory extends TableOfContentsFactory
         const elementId = await TableOfContentsUtils.Markdown.getHeadingId(
           this.parser!,
           heading.raw,
-          heading.level
+          heading.level,
+          this.sanitizer
         );
 
         if (!elementId) {
           return;
         }
-        const selector = `h${heading.level}[id="${elementId}"]`;
+        const selector = `h${heading.level}[id="${CSS.escape(elementId)}"]`;
 
         headingToElement.set(
           heading,

diff --git a/packages/notebook/src/toc.ts b/packages/notebook/src/toc.ts
@@ -588,10 +588,14 @@ export class NotebookToCFactory extends TableOfContentsFactory<NotebookPanel> {
 
     const findHeadingElement = (cell: Cell): void => {
       model.getCellHeadings(cell).forEach(async heading => {
-        const elementId = await getIdForHeading(heading, this.parser!);
+        const elementId = await getIdForHeading(
+          heading,
+          this.parser!,
+          this.sanitizer
+        );
 
         const selector = elementId
-          ? `h${heading.level}[id="${elementId}"]`
+          ? `h${heading.level}[id="${CSS.escape(elementId)}"]`
           : `h${heading.level}`;
 
         if (heading.outputIndex !== undefined) {
@@ -710,15 +714,17 @@ export class NotebookToCFactory extends TableOfContentsFactory<NotebookPanel> {
  */
 export async function getIdForHeading(
   heading: INotebookHeading,
-  parser: IRenderMime.IMarkdownParser
+  parser: IRenderMime.IMarkdownParser,
+  sanitizer: IRenderMime.ISanitizer
 ) {
   let elementId: string | null = null;
   if (heading.type === Cell.HeadingType.Markdown) {
     elementId = await TableOfContentsUtils.Markdown.getHeadingId(
       parser,
       // Type from TableOfContentsUtils.Markdown.IMarkdownHeading
       (heading as any).raw,
-      heading.level
+      heading.level,
+      sanitizer
     );
   } else if (heading.type === Cell.HeadingType.HTML) {
     // Type from TableOfContentsUtils.IHTMLHeading

diff --git a/packages/notebook/src/widget.ts b/packages/notebook/src/widget.ts
@@ -2191,14 +2191,15 @@ export class Notebook extends StaticNotebook {
               id = await TableOfContentsUtils.Markdown.getHeadingId(
                 this.rendermime.markdownParser!,
                 mdHeading.raw,
-                mdHeading.level
+                mdHeading.level,
+                this.rendermime.sanitizer
               );
             }
             break;
         }
         if (id === queryId) {
           const element = this.node.querySelector(
-            `h${heading.level}[id="${id}"]`
+            `h${heading.level}[id="${CSS.escape(id)}"]`
           ) as HTMLElement;
 
           return {

diff --git a/packages/rendermime-interfaces/src/index.ts b/packages/rendermime-interfaces/src/index.ts
@@ -486,10 +486,10 @@ export namespace IRenderMime {
    */
   export interface IMarkdownParser {
     /**
-     * Render a markdown source.
+     * Render a markdown source into unsanitized HTML.
      *
      * @param source - The string to render.
-     * @returns - A promise of the string.
+     * @returns - A promise of the string containing HTML which may require sanitization.
      */
     render(source: string): Promise<string>;
   }

diff --git a/packages/toc/package.json b/packages/toc/package.json
@@ -46,6 +46,7 @@
     "@jupyterlab/docregistry": "^4.0.10",
     "@jupyterlab/observables": "^5.0.10",
     "@jupyterlab/rendermime": "^4.0.10",
+    "@jupyterlab/rendermime-interfaces": "^3.8.10",
     "@jupyterlab/translation": "^4.0.10",
     "@jupyterlab/ui-components": "^4.0.10",
     "@lumino/coreutils": "^2.1.2",

diff --git a/packages/toc/src/utils/markdown.ts b/packages/toc/src/utils/markdown.ts
@@ -1,7 +1,9 @@
 // Copyright (c) Jupyter Development Team.
 // Distributed under the terms of the Modified BSD License.
 
+import { Sanitizer } from '@jupyterlab/apputils';
 import { IMarkdownParser, renderMarkdown } from '@jupyterlab/rendermime';
+import { IRenderMime } from '@jupyterlab/rendermime-interfaces';
 import { TableOfContents } from '../tokens';
 
 /**
@@ -24,27 +26,35 @@ export interface IMarkdownHeading extends TableOfContents.IHeading {
  *
  * @param raw Raw markdown heading
  * @param level Heading level
+ * @param sanitizer HTML sanitizer
  */
 export async function getHeadingId(
-  parser: IMarkdownParser,
+  markdownParser: IMarkdownParser,
   raw: string,
-  level: number
+  level: number,
+  sanitizer?: IRenderMime.ISanitizer
 ): Promise<string | null> {
   try {
-    const innerHTML = await parser.render(raw);
+    const host = document.createElement('div');
 
-    if (!innerHTML) {
-      return null;
-    }
+    await renderMarkdown({
+      markdownParser,
+      host,
+      source: raw,
+      trusted: false,
+      sanitizer: sanitizer ?? new Sanitizer(),
+      shouldTypeset: false,
+      resolver: null,
+      linkHandler: null,
+      latexTypesetter: null
+    });
 
-    const container = document.createElement('div');
-    container.innerHTML = innerHTML;
-    const header = container.querySelector(`h${level}`);
+    const header = host.querySelector(`h${level}`);
     if (!header) {
       return null;
     }
 
-    return renderMarkdown.createHeaderId(header);
+    return header.id;
   } catch (reason) {
     console.error('Failed to parse a heading.', reason);
   }

diff --git a/packages/toc/test/markdown.spec.ts b/packages/toc/test/markdown.spec.ts
@@ -2,9 +2,33 @@
 // Distributed under the terms of the Modified BSD License.
 
 import { TableOfContentsUtils } from '@jupyterlab/toc';
+import { Sanitizer } from '@jupyterlab/apputils';
+import { createMarkdownParser } from '@jupyterlab/markedparser-extension';
+import { IMarkdownParser } from '@jupyterlab/rendermime';
+import {
+  EditorLanguageRegistry,
+  IEditorLanguageRegistry
+} from '@jupyterlab/codemirror';
 
 describe('TableOfContentsUtils', () => {
   describe('Markdown', () => {
+    describe('#getHeadingId', () => {
+      const languages: IEditorLanguageRegistry = new EditorLanguageRegistry();
+      const parser: IMarkdownParser = createMarkdownParser(languages);
+      const sanitizer = new Sanitizer();
+      it.each<[string, string]>([
+        ['# Title', 'Title'],
+        [`# test'"></title><img>test {#'"><img>}`, `test'\">test-{#'\">}`]
+      ])('should derive ID from markdown', async (markdown, expectedId) => {
+        const headingId = await TableOfContentsUtils.Markdown.getHeadingId(
+          parser,
+          markdown,
+          1,
+          sanitizer
+        );
+        expect(headingId).toEqual(expectedId);
+      });
+    });
     describe('#getHeadings', () => {
       it.each<[string, TableOfContentsUtils.Markdown.IMarkdownHeading[]]>([
         [

diff --git a/packages/toc/tsconfig.json b/packages/toc/tsconfig.json
@@ -26,6 +26,9 @@
     },
     {
       "path": "../ui-components"
+    },
+    {
+      "path": "../rendermime-interfaces"
     }
   ]
 }
diff --git a/packages/toc/tsconfig.test.json b/packages/toc/tsconfig.test.json
@@ -26,6 +26,9 @@
     {
       "path": "../ui-components"
     },
+    {
+      "path": "../rendermime-interfaces"
+    },
     {
       "path": "."
     },

diff --git a/yarn.lock b/yarn.lock
@@ -4757,6 +4757,7 @@ __metadata:
     "@jupyterlab/docregistry": ^4.0.10
     "@jupyterlab/observables": ^5.0.10
     "@jupyterlab/rendermime": ^4.0.10
+    "@jupyterlab/rendermime-interfaces": ^3.8.10
     "@jupyterlab/testing": ^4.0.10
     "@jupyterlab/translation": ^4.0.10
     "@jupyterlab/ui-components": ^4.0.10