Add \class and \cssId on non-strict mode by ylemkimon · Pull Request #1437 · KaTeX/KaTeX

ylemkimon · 2018-06-17T04:09:30Z

Added string group type, which passes raw string argument

* Added `string` group type, which passes raw string argument

codecov · 2018-06-17T04:14:09Z

Codecov Report

Merging #1437 into master will decrease coverage by 0.37%.
The diff coverage is 44.11%.

@@            Coverage Diff            @@
##           master   #1437      +/-   ##
=========================================
- Coverage   82.57%   82.2%   -0.38%     
=========================================
  Files          77      78       +1     
  Lines        4408    4406       -2     
  Branches      772     771       -1     
=========================================
- Hits         3640    3622      -18     
- Misses        665     676      +11     
- Partials      103     108       +5

Impacted Files	Coverage Δ
src/buildHTML.js	`97.79% <ø> (ø)`	⬆️
src/functions.js	`100% <ø> (ø)`	⬆️
src/ParseNode.js	`88.88% <ø> (ø)`	⬆️
src/functions/href.js	`100% <100%> (ø)`	⬆️
src/Parser.js	`94.9% <100%> (+0.5%)`	⬆️
src/functions/html.js	`5% <5%> (ø)`

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 6195f2f...6ce0fde. Read the comment docs.

ylemkimon · 2018-06-17T04:21:47Z

src/Parser.js

-            if (lastToken.text === "{") {
-                nest += 1;
-            } else if (lastToken.text === "}") {
-                if (nest <= 0) {


This was unreachable, as while is terminated if nest === 0 && this.nextToken.text === groupEnd.

ronkok · 2018-06-17T13:29:19Z

As @kevinbarabash once reminded me, some people may want to protect their web site from users adding classes. Maybe this (terrific) code should go behind its own rendering option omitClass, default = true.

ry-randall · 2018-06-18T18:32:21Z

src/functions/html.js

+
+defineFunction({
+    type: "html",
+    names: ["\\class", "\\cssId"],


While I know mathjax calls this "cssId", I think it would be nice to give it a more generic name (i.e. "id"). My only reasoning is, if multiple renderers end up being supported, css may have no meaning (i.e. in a canvas).

For a similar reason, I would give this file a more generic name as well (i.e. identifiers.js)

@rrandallcainc were you thinking that id might be useful for hit testing in canvas based renderer.

@kevinbarabash Hit testing could be one use case. I was more thinking along the lines of being able to manipulate specific parts of the LaTeX separately. For example, let's say I had \frac{\id{numerator}{a}}{\id{denominator}{b}} and I wanted to switch the two. I see as id/class as general ways of targeting specific areas of the latex.

ry-randall · 2018-06-18T18:48:28Z

src/functions/html.js

+    names: ["\\class", "\\cssId"],
+    props: {
+        numArgs: 2,
+        argTypes: ["string", "original"],


I'm assuming we want this allowed in text?

@rrandallcainc Set allowedInText true.

Assuming you'll want to apply allowInText: true also to html, no?

ylemkimon · 2018-06-19T08:40:26Z

@ronkok I'm not sure how classes or ids can be abused, as styles are predefined.

ronkok · 2018-06-19T13:40:34Z

as styles are predefined.

Styles are predefined, that's true. But a malicious user, writing in an input box like the one I am writing in now, could mis-apply existing CSS style rules to places where they should not be. An administrator of a site like this one might want to restrict that.

@ylemkimon I think this PR is terrific, and I want to see it succeed. I only write to suggest a way that everybody can be happy with it.

ry-randall · 2018-06-21T14:04:53Z

Curious how "reserved" classes/id's are handled. For example, if someone tried \class{strut}{blah} would it apply the strut class unintentionally?

Also, do you plan on adding tests for this?

k4b7 · 2018-06-24T01:33:15Z

My guess is that class and cssId (or id) are commands that would be used by site/app developers and not something that really make sense for user submitted content so providing a way to disable these commands should be sufficient.

k4b7 · 2018-06-24T01:37:31Z

src/Parser.js

-    /**
-     * Parses a url string.
-     */
-    parseUrlGroup(optional: boolean): ?ParsedArg {


This change conflicts with #1440.

k4b7 · 2018-06-24T01:38:55Z

src/ParseNode.js

+    "html": {|
+        type: "html",
+        class?: string,
+        cssId?: string,


Maybe we could change this be:

type: "html" attribute: "class" | "id", value: string, body: AnyParseNode[],

@kevinbarabash I thought there may be use cases of setting both class and id.

But wouldn't there be separate commands, e.g. \class{foo}{\id{bar}{content}}?

k4b7 · 2018-06-24T01:39:25Z

src/ParseNode.js

+    "string": {|
+        type: "string",
+        value: string,
+    |},


Is this only used to replace url?

Also for the first argument of \cssId and \class.

k4b7 · 2018-06-24T01:42:24Z

src/functions/html.js

+            math.setAttribute('class', group.value.class);
+        }
+        if (group.value.cssId) {
+            math.setAttribute('id', group.value.cssId);


I've found that browsers tend to get confused about things when there are multiple elements with the same id.

Does it apply to the MathML elements, too?

If you query for an element by id you can get an HTML element, SVG element, MathML element, etc.

ylemkimon · 2018-06-26T18:27:30Z

@ronkok @ry-randall Another way of limiting their usage is allowing only those with specific prefix, like MathJax safe extension. What do you think?

ronkok · 2018-06-26T23:46:15Z

@ylemkimon I don't really have a strong personal opinion on this issue. I spoke up earlier to make you aware of a comment that @kevinbarabash had once made to me. Anything he likes on this issue will be okay with me, too.

ry-randall · 2018-06-27T17:27:43Z

@ylemkimon I'm not too familiar with the mathjax safe extension. Do you mean something like \class{katex-className}{blah} where "katex-" would be the safe extension?

Edit: I just took a look @ http://docs.mathjax.org/en/latest/options/other/Safe.html#configure-safe and found what you're referring to.

I think that's the right approach, for sure. Others thoughts?

edemaine · 2018-06-27T23:57:15Z

Hmm, related to #1436 then I guess. Is there a nice uniform solution? It'd be cool to be able to specify custom-* or something as allowed classes, or explicitly list allowed classes, though the "safe" specifier might also be sufficient in most cases.

edemaine · 2018-06-27T23:59:35Z

P.S. I do worry about defining an id that matches another id on the page, thereby hijacking some things tied to the page. It's not like the KaTeX object is going to do anything with that replacement (until we support \javascriptToRunWhenClicked{...}), but it could easy break existing code that uses document.getElementById. So some kind of limitation is needed.

k4b7 · 2018-06-29T03:32:18Z

So some kind of limitation is needed.

I think the limitation should be at the command level. We should disable \class and \id by default and allow users to opt-in by enabling "unsafe" mode. This should be relatively simple to do and allows users to move to the new version of KaTeX without opening a major attack vector. Users whose sites don't accept user input and who need \class and/or \id can turn of "unsafe" mode.

ronkok · 2018-06-29T03:45:43Z

I think the limitation should be at the command level.

What if a web site administrator wants protection from a malicious user? Would the limitation be better if set as a KaTeX rendering option?

edemaine · 2018-06-29T11:24:11Z

Perhaps we should add a new option safe or unsafe that is similar to strict, but is explicitly about security, and allows configuration (e.g. allow certain classes but not others, allow class but not id). The allowedProtocols configuration could be rolled in...?

k4b7 · 2018-06-29T19:27:36Z

I think the limitation should be at the command level.

What I meant by this is that I don't think we should try to filter things in/out by some id/class prefix. I think we should have a unsafe setting which is false by default which can be passed to the render function. Setting unsafe to true will allow the use of the \id and \class commands.

ylemkimon · 2018-07-03T08:16:42Z

I think we can go with RegExp | boolean, with default false. I'm afraid this will clutter settings, with a feature only few people will use. I think we should categorize settings and/or have a naming system.

k4b7 · 2018-07-04T00:46:43Z

@ylemkimon the boolean is whether or not class and id are allowed and the RegExp is what values they all?

k4b7 · 2018-07-04T00:47:32Z

Agree that a naming strategy for options/settings is a good idea.

k4b7 · 2018-07-22T02:12:26Z

For this to move forward it sounds like we need:

test cases involving \class{part-1}{\frac{1}{2} + \frac{3}{2}} = 2
changes to the code that address the spacing issues and make those test cases pass

ylemkimon · 2018-09-08T12:40:42Z

Blocked on #1706 and #1711.

dvergeylen · 2019-02-05T10:32:13Z

Hi @ylemkimon ,

As #1706 and #1711 are both merged by now, do you see anything blocking this?

Is #1771 blocking this or could a (disabled by default) option (e.g: {allowClass: false, allowId: false} ) be reasonable security setting?

I am currently needing this feature for a WYSIWYG editor, would be glad to give a hand!

edemaine · 2019-02-05T13:04:01Z

@dvergeylen Yes, this is blocked on #1771, or more precisely, #1794. We don't want to add a bunch of options and then switch to a different mechanic. Your feedback on #1794 would be appreciated!

steveluc · 2019-04-15T18:47:52Z

While we are waiting for #1794, is there a workaround to add styling to an element that corresponds to a substring of tex input? Id is overkill for this and so is class. For example, there could be something like \localId{id1}{x^2} which associates id1 with an element implementing the x^2 substring. We could have an option passed into render that supplies a map or object to be populated with these ids. Then whatever permission the page has to style elements, it can apply to these elements. Thanks.

k4b7 · 2019-04-17T12:58:48Z

I think this is still blocked, but by #1794 now.

k4b7 · 2019-07-09T01:59:35Z

This is no longer blocked.

k4b7 · 2019-09-22T19:15:01Z

Closing in preference of #2082.

ylemkimon added 2 commits June 17, 2018 13:08

Add \class and \cssId on non-strict mode

557ec0d

* Added `string` group type, which passes raw string argument

Merge branch 'master' into html-extension

54d95d4

Minor fixes

6bc1aae

ylemkimon commented Jun 17, 2018

View reviewed changes

ylemkimon added 2 commits June 17, 2018 13:37

Fix flow type

cd6c090

Fix flow type

ab23a61

ry-randall reviewed Jun 18, 2018

View reviewed changes

Merge branch 'master' into html-extension

7fa2f00

ry-randall reviewed Jun 18, 2018

View reviewed changes

Allow them in text mode

5d54c20

k4b7 requested changes Jun 24, 2018

View reviewed changes

Merge branch 'master' into html-extension

6ce0fde

ylemkimon mentioned this pull request Jul 16, 2018

Support \pmb #1418

Merged

k4b7 added the GH Review: needs-revision label Jul 22, 2018

ylemkimon added this to the Version 0.11.0 milestone Aug 8, 2018

ronkok mentioned this pull request Aug 9, 2018

Is there a way to pass "metadata" to render into HTML as classes? #1582

Closed

This was referenced Aug 13, 2018

Add \html command to insert HTML #1596

Closed

Support \includegraphics #1620

Merged

ylemkimon removed this from the Version 0.10.0 milestone Aug 18, 2018

ylemkimon mentioned this pull request Sep 7, 2018

Refactor buildExpression to depth-first traverse groups (nodes), fix spacings and \begingroup...\endgroup #1706

Merged

ylemkimon added the blocked label Sep 8, 2018

ylemkimon mentioned this pull request Sep 12, 2018

Add raw string group, move comment parsing to Parser, change URL group parser #1711

Merged

k4b7 removed the blocked label Oct 29, 2018

k4b7 assigned ylemkimon Oct 29, 2018

edemaine mentioned this pull request Nov 1, 2018

Security setting #1771

Closed

daniel3735928559 mentioned this pull request Nov 24, 2018

xmlClass function #1791

Closed

daniel3735928559 mentioned this pull request Dec 1, 2018

Update KaTeX to latest version daniel3735928559/guppy#92

Closed

k4b7 mentioned this pull request Mar 17, 2019

[PoC]Plugin architecture using webpack (HTML \class extension) #1074

Closed

4 tasks

ronkok mentioned this pull request Mar 31, 2019

Can a macro return html? #1916

Closed

k4b7 added the blocked label Apr 17, 2019

k4b7 removed the blocked label Jul 9, 2019

edemaine mentioned this pull request Jul 10, 2019

Support for \style{} #2029

Closed

ylemkimon mentioned this pull request Aug 20, 2019

Add HTML extension #2082

Merged

k4b7 closed this Sep 22, 2019

Uh oh!

Conversation

ylemkimon commented Jun 17, 2018

Uh oh!

codecov bot commented Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ylemkimon Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronkok commented Jun 17, 2018

Uh oh!

ry-randall Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ylemkimon commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronkok commented Jun 19, 2018

Uh oh!

ry-randall commented Jun 21, 2018

Uh oh!

k4b7 commented Jun 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ylemkimon commented Jun 26, 2018

Uh oh!

ronkok commented Jun 26, 2018

Uh oh!

ry-randall commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edemaine commented Jun 27, 2018

Uh oh!

edemaine commented Jun 27, 2018

Uh oh!

k4b7 commented Jun 29, 2018

Uh oh!

ronkok commented Jun 29, 2018

Uh oh!

edemaine commented Jun 29, 2018

Uh oh!

k4b7 commented Jun 29, 2018

Uh oh!

ylemkimon commented Jul 3, 2018

Uh oh!

k4b7 commented Jul 4, 2018

Uh oh!

k4b7 commented Jul 4, 2018

Uh oh!

codecov bot commented Jun 17, 2018 •

edited

Loading

ylemkimon Jun 17, 2018 •

edited

Loading

ry-randall Jun 18, 2018 •

edited

Loading

ylemkimon commented Jun 19, 2018 •

edited

Loading

ry-randall commented Jun 27, 2018 •

edited

Loading

ylemkimon commented Sep 8, 2018 •

edited

Loading

dvergeylen commented Feb 5, 2019 •

edited

Loading

k4b7 commented Apr 17, 2019 •

edited

Loading