Allow only allowed protocols in \href#1440
Allow only allowed protocols in \href#1440edemaine merged 7 commits intoKaTeX:masterfrom ylemkimon:href-protocol
Conversation
Added `allowedProtocols` settings
Codecov Report
@@ Coverage Diff @@
## master #1440 +/- ##
==========================================
+ Coverage 83.2% 83.22% +0.01%
==========================================
Files 77 77
Lines 4417 4422 +5
Branches 772 773 +1
==========================================
+ Hits 3675 3680 +5
Misses 642 642
Partials 100 100
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Nice idea. It seems good in particular to forbid blob: and file: and especially javascript: by default, and give people an option to customize. I just have a few small comments.
One bigger question: Should there be an option to allow all protocols, when the source is fully trusted? (There's already a way to forbid all protocols: [].) I see two extensions that would achieve this:
- Also allow a function for
allowedProtocolsthat decides whether to allow a given protocol. Then(protocol) => truewould allow all. - Also allow a
trueargument to allow all protocols. This may be less self-explanatory.
Do you think this is worth adding? Or should we stick to an explicit whitelist like most sanitizers?
test/katex-spec.js
Outdated
| }); | ||
|
|
||
| it("should not allow protocols not in allowedProtocols", function() { | ||
| expect("\\href{javscript:alert('x')}{foo}").toNotParse(); |
There was a problem hiding this comment.
This should probably be javascript:.
| } else if (protocol == null && this.settings.allowedProtocols.indexOf( | ||
| "_relative") === -1) { | ||
| throw new ParseError('Not allowed relative url in \\href', res); | ||
| } |
There was a problem hiding this comment.
You could use utils.contains which is designed for this purpose (and equivalent). I don't feel strongly about this, though... I've always found that helper a bit small to be important.
README.md
Outdated
| - `colorIsTextColor`: `boolean`. If `true`, `\color` will work like LaTeX's `\textcolor`, and take two arguments (e.g., `\color{blue}{hello}`), which restores the old behavior of KaTeX (pre-0.8.0). If `false` (the default), `\color` will work like LaTeX's `\color`, and take one argument (e.g., `\color{blue}hello`). In both cases, `\textcolor` works as in LaTeX (e.g., `\textcolor{blue}{hello}`). | ||
| - `maxSize`: `number`. All user-specified sizes, e.g. in `\rule{500em}{500em}`, will be capped to `maxSize` ems. If set to `Infinity` (the default), users can make elements and spaces arbitrarily large. | ||
| - `maxExpand`: `number`. Limit the number of macro expansions to the specified number, to prevent e.g. infinite macro loops. If set to `Infinity`, the macro expander will try to fully expand as in LaTeX. (default: 1000) | ||
| - `allowedProtocols`: `string[]`. Allowed protocols in `\href`. Use `_relative` to allow relative urls. (default : `["http", "https", "mailto", "_relative"]`) |
There was a problem hiding this comment.
Please remove the space after default.
|
@edemaine Thank you for the review! I've added boolean argument to allow or disallow all protocols. I'm not sure about the function. |
|
One alternative solution might be to allow |
|
@kevinbarabash That might be considered more standard. I could go either way. @ylemkimon Thoughts? |
Revert 'Allow boolean argument'
|
I think |
|
Looks great! |
|
I wonder this qualifies as a security vulnerability. |
|
Maybe, given that |
|
We can add a |
allowedProtocolssettings