Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the kramdown math engine to be overridden #545

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stevenkaras
Copy link

This will allow the use of the nil math engine in kramdown, which
provides a saner default for templates that don't want mathjax but want
a safe fallback for when javascript is disabled.

From the history of this repo, the math engine used to be changeable up until 2 years ago when kramdown became the primary/only markdown engine. I have not checked if the version of kramdown used back then even had multiple math engines.

This will allow the use of the nil math engine in kramdown, which
provides a saner default for templates that don't want mathjax but want
a safe fallback for when javascript is disabled.
@aterenin
Copy link

+1. Please allow this the math engine to be disabled completely. I use KaTeX, which is lighter and substantially preferable than MathJax for my site.

@parkr
Copy link
Contributor

parkr commented Jul 2, 2018

Hey! I'd be happy to accept this PR with the following condition: we override any non-nil math engine to mathjax. We cannot permit server-side processing of kramdown's math engines at this time, so setting math_engine: katex should be overwritten to mathjax. If math_engine: null, then we can leave it.

@parkr parkr requested a review from benbalter July 2, 2018 18:52
Copy link
Contributor

@parkr parkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requires code which overwrites non-nil values, e.g.

config["kramdown"]["math_engine"] = DEFAULTS["kramdown"]["math_engine"] unless config["kramdown"]["math_engine"].nil?

@stevenkaras
Copy link
Author

The docs seem to indicate that the KaTeX engine is intended for untrusted user input. Of course, the PR as it stands now would also allow the sskatex engine, which is explicitly insecure.

Perhaps a better way would be to list permitted options, with the first as a default? That would be a more flexible solution moving forward, and keep the current declarative style.

@mahrud
Copy link

mahrud commented Jun 4, 2019

@stevenkaras, any progress on this?

I also prefer allowing katex as an option so clients don't need to have javascript enabled, but if it requires a lengthy audit of that gem by GitHub, I'd take the nil-or-mathjax solution to no solution at all.

@parkr
Copy link
Contributor

parkr commented Jun 4, 2019

I don't think we can do any security auditing of new gems, so I'd like to simply allow you to unset the math engine, i.e. allowed values are nil and mathjax. This is due to amount of time that goes into auditing these new gems for untrusted user input. We have to do our due diligence to keep our users safe, and we can't commit to that at this time.

Allowing the math_engine to be nil is fine by me.

@mahrud
Copy link

mahrud commented Jun 4, 2019

That sounds reasonable. I don't have access to this PR, so opened a new one off of master with your requested changes.

@benbalter benbalter removed their request for review October 29, 2019 13:49
mahrud added a commit to mahrud/pages-gem that referenced this pull request Apr 15, 2020
mahrud added a commit to mahrud/pages-gem that referenced this pull request Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants