Repository: Khan/KaTeX
Pull request #1169 (2018-02-18 17:47:20)

Stop throwing ParseError when throwOnError is false
This PR is motivated by the recent security issue reported in #1160. I would argue that almost anyone who uses KaTeX on a user-specified string doesn't want an exception to happen, but rather wants KaTeX to do its best job at rendering the given LaTeX.  `throwOnError` is the obvious "solution", but it actually only fixes invalid commands, but any other type of `ParseError` still gets thrown.  This is nice because you get partial renderings, but not nice because it doesn't fix the original problem of not throwing exceptions.  As a result, I believe many KaTeX users (myself included) write our own `try ... catch ...` block to render something else when KaTeX throws an error.  And if done poorly, this leads to security holes like #1160.

This PR makes `throwOnError` more useful by rendering HTML for all types of `ParseError`.  If it's just an unknown command, you get a partial render as before.  But if you have some other type of error, you get a `<span class="katex-error">` whose text is the LaTeX source code, its color is `options.errorColor` (defaulting to red as usual), and it has a hover title that displays the actual error message.  This is what I did in [my own KaTeX error handler](https://github.com/edemaine/coauthor/blob/8a2f9c5f19591d04a83e1fa26d74c5b1fc0c0e71/lib/formats.coffee#L437-L453).  Crucially, the output HTML is properly escaped (thanks to our existing domTree infrastructure -- see the test snapshot).

There's an obvious alternative: show the error message directly, and hover to see the LaTeX. Or some other combination.  But IMHO this PR is probably closest to the intent of the author: if you can't render nice LaTeX, at least render the source code.

Another question is naming.  I took over `throwOnError` because it is the natural name for this feature (and probably what most people expect).  But it breaks backwards compatibility, so would need to be a new version.  We could use another name to keep backward compatibility.  We could also provide the old behavior, via something like `throwOnBadCommand`.  I'm not sure of a great use for this...  If one wants to reformat the error style, they could use CSS, or (in the worst case) parse the resulting `<span class="katex-error">` and restructure.

Along the way:
* Use new `katex.__renderToDomTree` in katex-spec's `_getBuilt`.  (This was necessary to get the new error handling in `_getBuilt`.)
* Fix jest results which must always be functions, not strings.

codecov[bot]: # [Codecov](https://codecov.io/gh/Khan/KaTeX/pull/1169?src=pr&el=h1) Report
> Merging [#1169](https://codecov.io/gh/Khan/KaTeX/pull/1169?src=pr&el=desc) into [master](https://codecov.io/gh/Khan/KaTeX/commit/0854bb9f1579f5f084a69d9483f6e22debb64f01?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `73.33%`.


[![Impacted file tree graph](https://codecov.io/gh/Khan/KaTeX/pull/1169/graphs/tree.svg?height=150&width=650&token=vfDnj2P77o&src=pr)](https://codecov.io/gh/Khan/KaTeX/pull/1169?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1169      +/-   ##
==========================================
+ Coverage   79.38%   79.39%   +<.01%     
==========================================
  Files          59       59              
  Lines        3871     3882      +11     
  Branches      648      649       +1     
==========================================
+ Hits         3073     3082       +9     
- Misses        663      665       +2     
  Partials      135      135
```


| [Impacted Files](https://codecov.io/gh/Khan/KaTeX/pull/1169?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [katex.js](https://codecov.io/gh/Khan/KaTeX/pull/1169/diff?src=pr&el=tree#diff-a2F0ZXguanM=) | `67.64% <73.33%> (+6.77%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Khan/KaTeX/pull/1169?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Khan/KaTeX/pull/1169?src=pr&el=footer). Last update [0854bb9...da91f3c](https://codecov.io/gh/Khan/KaTeX/pull/1169?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ylemkimon: How about using error level and deprecating `throwOnError`:
0 - suppress all errors, instead print LaTeX source code, this PR's behavior when `!throwOnError`
1 - suppress all errors except unsupported command error, current behavior when `!throwOnError`
2 - throw all errors, current/PR behavior when `throwOnError`
kevinbarabash: @edemaine thanks for the PR.  This is definitely a pain point for users.  I will review this weekend.
kevinbarabash: > How about using error levels:
0 - suppress all errors, instead print LaTeX source code, this PR's behavior when !throwOnError
1 - suppress unsupported command error, current behavior when !throwOnError
2 - throw all errors, current/PR behavior when throwOnError

I can't think of a situation where people would actually want level 1.  They'd still need to use a try/catch to avoid errors other than unsupported command errors.
ylemkimon: @kevinbarabash A user may want to implement custom error handler, but in the custom error handler, it cannot access KaTeX internals, i.e., if it's unsupported command error, print the plain command name, though as you said, I'm not sure there are people want it.
edemaine: I could see wanting to build a custom handler for custom formatting in the error case.  An alternative would be to provide a callback to call instead of an error handler.  Maybe `throwOnError` could receive a function argument to handle this case?
ylemkimon: This will also fix #475.

Related: #538
ylemkimon: With #1226, I think we should have an error callback and handle errors differently in there.
kevinbarabash: @edemaine looks like there's some lint:
```
/home/travis/build/Khan/KaTeX/test/katex-spec.js
  3120:16  error  'buildTree' is not defined  no-undef
```
edemaine: Thanks for the review, @ylemkimon. Do you think I should add an error callback function for custom error handling/rendering? Or leave that for consideration in another PR?
ylemkimon: Now I think currently there is no way to access KaTeX internals, and custom error handler probably should be another issue/PR. Thank you for the PR!

Patch:
diff --git a/katex.js b/katex.js
index 7b9700e8..89073210 100644
--- a/katex.js
+++ b/katex.js
@@ -13,6 +13,8 @@ import Settings from "./src/Settings";
 
 import { buildTree, buildHTMLTree } from "./src/buildTree";
 import parseTree from "./src/parseTree";
+import buildCommon from "./src/buildCommon";
+import domTree from "./src/domTree";
 import utils from "./src/utils";
 
 import type {SettingsOptions} from "./src/Settings";
@@ -72,6 +74,26 @@ const generateParseTree = function(
     return parseTree(expression, settings);
 };
 
+/**
+ * If the given error is a KaTeX ParseError and options.throwOnError is false,
+ * renders the invalid LaTeX as a span with hover title giving the KaTeX
+ * error message.  Otherwise, simply throws the error.
+ */
+const renderError = function(
+    error,
+    expression: string,
+    options: Settings,
+) {
+    if (options.throwOnError || !(error instanceof ParseError)) {
+        throw error;
+    }
+    const node = buildCommon.makeSpan(["katex-error"],
+        [new domTree.symbolNode(expression)]);
+    node.setAttribute("title", error.toString());
+    node.setAttribute("style", `color:${options.errorColor}`);
+    return node;
+};
+
 /**
  * Generates and returns the katex build tree. This is used for advanced
  * use cases (like rendering to custom output).
@@ -81,8 +103,12 @@ const renderToDomTree = function(
     options: SettingsOptions,
 ) {
     const settings = new Settings(options);
-    const tree = parseTree(expression, settings);
-    return buildTree(tree, expression, settings);
+    try {
+        const tree = parseTree(expression, settings);
+        return buildTree(tree, expression, settings);
+    } catch (error) {
+        return renderError(error, expression, settings);
+    }
 };
 
 /**
@@ -94,8 +120,12 @@ const renderToHTMLTree = function(
     options: SettingsOptions,
 ) {
     const settings = new Settings(options);
-    const tree = parseTree(expression, settings);
-    return buildHTMLTree(tree, expression, settings);
+    try {
+        const tree = parseTree(expression, settings);
+        return buildHTMLTree(tree, expression, settings);
+    } catch (error) {
+        return renderError(error, expression, settings);
+    }
 };
 
 export default {
diff --git a/static/main.js b/static/main.js
index 896caa42..01e72df5 100644
--- a/static/main.js
+++ b/static/main.js
@@ -14,7 +14,7 @@ function init() {
     input.addEventListener("input", reprocess, false);
     permalink.addEventListener("click", setSearch);
 
-    const options = {displayMode: true, macros: {}};
+    const options = {displayMode: true, throwOnError: false, macros: {}};
     const query = queryString.parse(window.location.search);
 
     if (query.text) {


Test patch:
diff --git a/test/__snapshots__/katex-spec.js.snap b/test/__snapshots__/katex-spec.js.snap
index 2afc94dd..612fa342 100644
--- a/test/__snapshots__/katex-spec.js.snap
+++ b/test/__snapshots__/katex-spec.js.snap
@@ -1,5 +1,39 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`A parser that does not throw on unsupported commands should build katex-error span for other type of KaTeX error 1`] = `
+{
+  "attributes": {
+    "style": "color:#933",
+    "title": "ParseError: KaTeX parse error: Double superscript at position 4: 2^2^̲2"
+  },
+  "children": [
+    {
+      "classes": [
+      ],
+      "depth": 0,
+      "height": 0,
+      "italic": 0,
+      "maxFontSize": 0,
+      "skew": 0,
+      "style": {
+      },
+      "value": "2^2^2",
+      "width": 0
+    }
+  ],
+  "classes": [
+    "katex-error"
+  ],
+  "depth": 0,
+  "height": 0,
+  "maxFontSize": 0,
+  "style": {
+  }
+}
+`;
+
+exports[`A parser that does not throw on unsupported commands should properly escape LaTeX in errors 1`] = `"<span class=\\"katex-error\\" title=\\"ParseError: KaTeX parse error: Expected group after &#x27;^&#x27; at position 2: 2^̲&amp;&quot;&lt;&gt;\\" style=\\"color:#933\\">2^&amp;&quot;&lt;&gt;</span>"`;
+
 exports[`An implicit group parser within optional groups should work style commands \\sqrt[\\textstyle 3]{x} 1`] = `
 [
   {
diff --git a/test/katex-spec.js b/test/katex-spec.js
index cae86e94..9c4c6fdb 100644
--- a/test/katex-spec.js
+++ b/test/katex-spec.js
@@ -44,8 +44,11 @@ const defaultOptions = new Options({
 
 const _getBuilt = function(expr, settings) {
     const usedSettings = settings ? settings : defaultSettings;
-    const parsedTree = parseTree(expr, usedSettings);
-    const rootNode = buildTree(parsedTree, expr, usedSettings);
+    const rootNode = katex.__renderToDomTree(expr, usedSettings);
+
+    if (rootNode.classes.indexOf('katex-error') >= 0) {
+        return rootNode;
+    }
 
     // grab the root node of the HTML rendering
     const builtHTML = rootNode.children[1];
@@ -98,10 +101,10 @@ const parseAndSetResult = function(expr, result, settings) {
     } catch (e) {
         result.pass = false;
         if (e instanceof ParseError) {
-            result.message = "'" + expr + "' failed " +
+            result.message = () => "'" + expr + "' failed " +
                 "parsing with error: " + e.message;
         } else {
-            result.message = "'" + expr + "' failed " +
+            result.message = () => "'" + expr + "' failed " +
                 "parsing with unknown error: " + e.message;
         }
     }
@@ -113,10 +116,10 @@ const buildAndSetResult = function(expr, result, settings) {
     } catch (e) {
         result.pass = false;
         if (e instanceof ParseError) {
-            result.message = "'" + expr + "' failed " +
+            result.message = () => "'" + expr + "' failed " +
                 "parsing with error: " + e.message;
         } else {
-            result.message = "'" + expr + "' failed " +
+            result.message = () => "'" + expr + "' failed " +
                 "parsing with unknown error: " + e.message;
         }
     }
@@ -149,10 +152,10 @@ beforeEach(function() {
             } catch (e) {
                 if (e instanceof ParseError) {
                     result.pass = true;
-                    result.message = "'" + actual + "' correctly " +
+                    result.message = () => "'" + actual + "' correctly " +
                         "didn't parse with error: " + e.message;
                 } else {
-                    result.message = "'" + actual + "' failed " +
+                    result.message = () => "'" + actual + "' failed " +
                         "parsing with unknown error: " + e.message;
                 }
             }
@@ -175,10 +178,10 @@ beforeEach(function() {
             } catch (e) {
                 result.pass = false;
                 if (e instanceof ParseError) {
-                    result.message = "'" + actual + "' failed to " +
+                    result.message = () => "'" + actual + "' failed to " +
                         "build with error: " + e.message;
                 } else {
-                    result.message = "'" + actual + "' failed " +
+                    result.message = () => "'" + actual + "' failed " +
                         "building with unknown error: " + e.message;
                 }
             }
@@ -2638,6 +2641,18 @@ describe("A parser that does not throw on unsupported commands", function() {
         expect(parsedInput[0].type).toBe("color");
         expect(parsedInput[0].value.color).toBe(errorColor);
     });
+
+    it("should build katex-error span for other type of KaTeX error", function() {
+        // Use _getBuilt instead of getBuilt to avoid calling expect...toParse
+        // and thus throwing parse error
+        const built = _getBuilt("2^2^2", noThrowSettings);
+        expect(built).toMatchSnapshot();
+    });
+
+    it("should properly escape LaTeX in errors", function() {
+        const html = katex.renderToString("2^&\"<>", noThrowSettings);
+        expect(html).toMatchSnapshot();
+    });
 });
 
 describe("The symbol table integrity", function() {