Stop throwing ParseError when throwOnError is false #1169

edemaine · 2018-02-18T17:47:20Z

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. 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.

`render`, `renderToString`, etc. now catch ParseError and render it to the raw LaTeX (with proper escaping) and a hover title with the error. 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 · 2018-02-18T17:50:46Z

Codecov Report

Merging #1169 into master will increase coverage by 0.01%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #1169      +/-   ##
==========================================
+ Coverage   83.76%   83.78%   +0.01%     
==========================================
  Files          61       61              
  Lines        3978     3989      +11     
  Branches      662      663       +1     
==========================================
+ Hits         3332     3342      +10     
- Misses        547      548       +1     
  Partials       99       99

Impacted Files	Coverage Δ
katex.js	`79.41% <93.33%> (+5.49%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e529dd...761cbe6. Read the comment docs.

Leave error type unspecified, as we check it with instanceof.

ylemkimon · 2018-03-09T07:16:42Z

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

kevinbarabash · 2018-03-09T16:37:44Z

@edemaine thanks for the PR. This is definitely a pain point for users. I will review this weekend.

kevinbarabash · 2018-03-09T16:42:05Z

test/katex-spec.js

@@ -98,10 +100,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;


Why did result.message change to a callback?

This is necessary since some update to Jest: Jest requires all messages to be callable. Obviously not directly related to this PR, but I ran into this (presumably long-standing) bug when my tests were failing.

kevinbarabash · 2018-03-09T16:43:05Z

test/__snapshots__/katex-spec.js.snap

+`;
+
+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>"`;
+


kevinbarabash · 2018-03-09T16:46:57Z

static/main.js

@@ -21,7 +21,7 @@ function init() {
    const macros = {};
    // TODO: Add toggle for displayMode.
    // https://github.com/Khan/KaTeX/issues/1035
-    const options = {displayMode: true};
+    const options = {displayMode: true, throwOnError: false};


kevinbarabash · 2018-03-09T16:48:05Z

katex.js

+    options: Settings,
+) {
+    if (options.throwOnError || !(error instanceof ParseError)) {
+        throw error;


Does re-throwing maintain the call stack?

@kevinbarabash Yes 😄

kevinbarabash · 2018-03-09T16:48:21Z

katex.js

+ * renders the invalid LaTeX as a span with hover title giving the KaTeX
+ * error message.  Otherwise, simply throws the error.
+ */
+const renderError = function(


This is a really slick solution.

kevinbarabash · 2018-03-11T22:20:05Z

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 · 2018-03-12T01:48:54Z

@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 · 2018-03-12T16:06:30Z

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 · 2018-03-16T15:22:17Z

This will also fix #475.

Related: #538

ylemkimon · 2018-03-22T16:05:57Z

With #1226, I think we should have an error callback function(handler) and handle errors accordingly.

kevinbarabash · 2018-04-04T01:34:43Z

@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 · 2018-05-09T13:09:19Z

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 · 2018-05-09T13:17:59Z

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!

edemaine · 2018-05-09T13:19:54Z

Sounds good! By the way, this is a breaking change compared to old behavior (rendering instead of throwing an exception), so should we advance to 0.10? @kevinbarabash

PR KaTeX#1169 forgot to document itself. This fixes the README accordingly.

PR #1169 forgot to document itself. This fixes the README accordingly.

kevinbarabash and others added 3 commits February 18, 2018 13:20

fix lint

fb79fc9

Merge branch 'master' into throwonerror

ae79b0e

Fix flow error

fc7d085

Leave error type unspecified, as we check it with instanceof.

kevinbarabash reviewed Mar 9, 2018

View reviewed changes

Merge branch 'master' into throwonerror

4adab08

edemaine mentioned this pull request Mar 16, 2018

Update docs to include try/catch in examples #275

Closed

kevinbarabash added the GH Review: review-needed label Apr 21, 2018

ylemkimon added 2 commits May 9, 2018 21:52

Merge branch 'master' into throwonerror

5102d23

Update katex-spec.js

761cbe6

ylemkimon approved these changes May 9, 2018

View reviewed changes

ylemkimon merged commit 49e673a into KaTeX:master May 9, 2018

edemaine mentioned this pull request May 9, 2018

throwOnError confusing #475

Closed

edemaine added a commit to edemaine/KaTeX that referenced this pull request May 9, 2018

Document new error handling behavior (renderOnError)

c8e5b8d

PR KaTeX#1169 forgot to document itself. This fixes the README accordingly.

edemaine mentioned this pull request May 9, 2018

Document new error handling behavior (renderOnError) #1302

Merged

ylemkimon pushed a commit that referenced this pull request May 9, 2018

Document new error handling behavior (renderOnError) (#1302)

3613885

PR #1169 forgot to document itself. This fixes the README accordingly.

ylemkimon mentioned this pull request Aug 18, 2018

Release v0.10.0-rc #1634

Merged

1 task

ghost mentioned this pull request Nov 7, 2018

gatsby-remark-katex: Add some options to render function gatsbyjs/gatsby#9740

Closed

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade katex from 0.9.0 to 0.11.1 saurabharch/Rocket.Chat#1

Open

edemaine mentioned this pull request Aug 9, 2022

Option like throwOnError: false to suppress all errors, not just ParseError #3680

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop throwing ParseError when throwOnError is false #1169

Stop throwing ParseError when throwOnError is false #1169

edemaine commented Feb 18, 2018

codecov bot commented Feb 18, 2018 •

edited

Loading

ylemkimon commented Mar 9, 2018 •

edited

Loading

kevinbarabash commented Mar 9, 2018

kevinbarabash Mar 9, 2018

edemaine Mar 14, 2018

kevinbarabash Mar 9, 2018

kevinbarabash Mar 9, 2018

kevinbarabash Mar 9, 2018

ylemkimon Mar 11, 2018

kevinbarabash Mar 9, 2018

kevinbarabash commented Mar 11, 2018

ylemkimon commented Mar 12, 2018 •

edited

Loading

edemaine commented Mar 12, 2018

ylemkimon commented Mar 16, 2018

ylemkimon commented Mar 22, 2018 •

edited

Loading

kevinbarabash commented Apr 4, 2018

edemaine commented May 9, 2018

ylemkimon commented May 9, 2018

edemaine commented May 9, 2018 •

edited

Loading

		`;

		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 '^' at position 2: 2^̲&"<>\\" style=\\"color:#933\\">2^&"<></span>"`;

Stop throwing ParseError when throwOnError is false #1169

Stop throwing ParseError when throwOnError is false #1169

Conversation

edemaine commented Feb 18, 2018

codecov bot commented Feb 18, 2018 • edited Loading

Codecov Report

ylemkimon commented Mar 9, 2018 • edited Loading

kevinbarabash commented Mar 9, 2018

kevinbarabash Mar 9, 2018

Choose a reason for hiding this comment

edemaine Mar 14, 2018

Choose a reason for hiding this comment

kevinbarabash Mar 9, 2018

Choose a reason for hiding this comment

kevinbarabash Mar 9, 2018

Choose a reason for hiding this comment

kevinbarabash Mar 9, 2018

Choose a reason for hiding this comment

ylemkimon Mar 11, 2018

Choose a reason for hiding this comment

kevinbarabash Mar 9, 2018

Choose a reason for hiding this comment

kevinbarabash commented Mar 11, 2018

ylemkimon commented Mar 12, 2018 • edited Loading

edemaine commented Mar 12, 2018

ylemkimon commented Mar 16, 2018

ylemkimon commented Mar 22, 2018 • edited Loading

kevinbarabash commented Apr 4, 2018

edemaine commented May 9, 2018

ylemkimon commented May 9, 2018

edemaine commented May 9, 2018 • edited Loading

codecov bot commented Feb 18, 2018 •

edited

Loading

ylemkimon commented Mar 9, 2018 •

edited

Loading

ylemkimon commented Mar 12, 2018 •

edited

Loading

ylemkimon commented Mar 22, 2018 •

edited

Loading

edemaine commented May 9, 2018 •

edited

Loading