KaTeX icon indicating copy to clipboard operation
KaTeX copied to clipboard

feat: add \htmlAttr command

Open junghoocho opened this issue 4 years ago • 9 comments

This pull request is related to issue #2265. In #2265, I suggested adding \htmlAttr, so that we can add any HTML attributes into a single enclosing <span> tag. This pull request includes the \htmlAttr that I suggested in #2265. My implementation is a very minor modification of the existing \htmlData command.

junghoocho avatar Jun 04 '20 09:06 junghoocho

Hey @junghoocho,

Thanks for the PR! Mind signing our Contributor License Agreement?

When you've done so, go ahead and comment [clabot:check] and I'll check again.

Yours truly, khanbot

khanbot avatar Jun 04 '20 09:06 khanbot

[clabot:check]

junghoocho avatar Jun 04 '20 09:06 junghoocho

CLA signature looks good :+1:

khanbot avatar Jun 04 '20 09:06 khanbot

If this command is considered too dangerous because it allows an arbitrary attribute to be inserted, we can add an explicit check of the added attribute names, so that only id, class, style, and data- attributes are added. What I hope to get is to have a mechanism so that all needed attributes are included in a single enclosing span tag.

junghoocho avatar Jun 04 '20 10:06 junghoocho

I looked at the MDN doc on the span element to see on what attributes an attacker may potentially insert JavaScript code. From my review, it seems clear that event-handler attributes with names like onX are the only places with potential JavaScript code. In my third commit 4bc5626d9508b92d4ba66c63f6dd6daa496d9d0f, I added an explicit check to disallow setting any attributes starting with on via \htmlAttr. Of course, since \htmlAttr has its own TrustContext, people can configure exactly what attributes are allowed through the command, but I suspect we don't want to allow inserting JavaScript through KaTeX command under any scenario. I hope this will make the addition of \htmlAttr safe enough to be merged to the mainline.

junghoocho avatar Jun 06 '20 20:06 junghoocho

I'm against adding \htmlAttr as (1) validation of attributes is impossible and (2) it may open to vulnerabilities and blacklisting is not enough.

ylemkimon avatar Jul 12 '20 07:07 ylemkimon

To make \htmlAttr logically equivalent to the union of existing \htmlId``\htmlClass \htmlStyle, and \htmlData commands, I can add the following validation logic to \htmlAttr:

(1) limit allowed attribute names to id, class, style,data-* (2) for id, class, style, data-*, let the attribute values pass the existing validation logics for htmlid, htmlclass and htmlstyle, htmldata values, respectively.

If you think this will be an acceptable implementation, I will be happy to add the above logic to the pull request for review.

junghoocho avatar Jul 13 '20 20:07 junghoocho

@junghoocho That seems to be OK. If you don't mind, could you update the PR? I'll leave #2265 open for discussions on allowing other attributes.

ylemkimon avatar Jul 17 '20 14:07 ylemkimon

@ylemkimon Thank you very much. I just updated the pull request to allow only id, style, class, and data- attributes in \htmlAttr

Regarding attribute-value validation, I find that the existing code does not perform any validation on its own and relies on the user-provided trust function. Given this, the updated pull request simply calls the user-provided trust function for validation as well.

Let me know if it looks OK or if I missed anything. Thank you.

junghoocho avatar Jul 18 '20 02:07 junghoocho