-
Notifications
You must be signed in to change notification settings - Fork 676
Refactor solution webview to reuse markdown engine #224
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
Conversation
src/webview/markdownEngine.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain more why we need the validator here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the routine adopted by built-in markdown exntesion's MarkdownEngine, I think it may enables us to validate local file link, which may be useful in local debugger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok... Actually still not quite understand... I think it's a worth investigating topic. It's fine to leave it here in this PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document from markdown-it:
MarkdownIt#validateLink(url)Boolean
Link validation function. CommonMark allows too much in links. By default we disable javascript:, vbscript:, file: schemas, and almost all data:... schemas except some embedded image types.
You can change this behaviour:
var md = require('markdown-it')(); // enable everything md.validateLink = function () { return true; }
Since we will deal with local files in WebView, chances are that it will be helpful to enable file: link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into the source code, and found that it was made to prevent XSS attack. Here are some examples:
- When parsing image and link:
if (res.ok) { href = state.md.normalizeLink(res.str); if (state.md.validateLink(href)) { pos = res.pos; } else { href = ''; } }
If validateLink does not pass, then href is screened out.
- The same goes on for reference-link or auto link:
href = state.md.normalizeLink(res.str); if (!state.md.validateLink(href)) { return false; }
So, it is indeed necessary to loose the restriction to allow file:// protocol.
Now the markdown engine is fully decoupled with webviews with the help of env parameter:
const body: string = this.markdown.render(solution.body, { lang: this.solution.lang, host: "https://discuss.leetcode.com/", });
src/webview/markdownEngine.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok... Actually still not quite understand... I think it's a worth investigating topic. It's fine to leave it here in this PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much much better. Just some small change requests.
Thank you!
Introduction
MarkdownEngineclass.Details
file://protocal.