Introduce new summary component.#695
Conversation
Summary component is intended for rendering markdown (MD) files.
✅ Deploy Preview for k8s-prow ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
area/deck
|
/retest |
michelle192837
left a comment
michelle192837
left a comment
There was a problem hiding this comment.
Thanks! Looks good to me for implementation (though I'd feel better with another maintainer's approval, @petr-muller or another if you'd like to take a look).
/lgtm
/hold
(in case Petr wants to take a look)
| continue | ||
| } | ||
|
|
||
| htmlContent := blackfriday.Run(content) |
There was a problem hiding this comment.
note: I believe this is a case where we'd want to consider sanitizing the input, but executeTemplate() later should take care of that, so this should be fine.
There was a problem hiding this comment.
I think it's not fine, the output is fed to template.HTML below which is documented to assume the input to be safe:
HTML encapsulates a known safe HTML document fragment. It should not be used for HTML from a third-party
I guess we could use it this way if the markdown processor is trusted to emit safe HTML from potentially unsafe markdown, but the blackfriday one does not seem to do that at least by default:
Blackfriday itself does nothing to protect against malicious content. If you are dealing with user-supplied markdown, we recommend running Blackfriday's output through HTML sanitizer such as Bluemonday.
There was a problem hiding this comment.
FWIW the goldmark processor claims to be safe:
By default, goldmark does not render raw HTML or potentially dangerous links
There was a problem hiding this comment.
Goldmark seems reasonable 👍
I think you're right that template.HTML isn't safe, but I think ExecuteTemplate() is? (Though it is parsing arbitrary content, so the "template author is trusted" assumption isn't quite there).
There was a problem hiding this comment.
😅 I'm just roleplaying a web developer here, it is possible that I'm wrong :D
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
/assign @petr-muller |
|
Hi @petr-muller, Could you take a look at my PR? Thank you! |
petr-muller
left a comment
petr-muller
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I thought about having a Markdown lens in the past, I believe it can be very useful.
Some high-level notes:
- Should it just be called
markdownlens instead of "summary"? - I'm no expert on Markdown Go libraries, but https://github.com/russross/blackfriday seems essentially abandoned; is there a reason why you chose it? Quick google search showed https://github.com/yuin/goldmark as an actively developed candidate but I really did not do any deeper research.
- The logic is fairly simple but I still think we'd benefit from having basic tests. It makes it easier for folks to extend the behavior in the future.
| continue | ||
| } | ||
|
|
||
| htmlContent := blackfriday.Run(content) |
There was a problem hiding this comment.
I think it's not fine, the output is fed to template.HTML below which is documented to assume the input to be safe:
HTML encapsulates a known safe HTML document fragment. It should not be used for HTML from a third-party
I guess we could use it this way if the markdown processor is trusted to emit safe HTML from potentially unsafe markdown, but the blackfriday one does not seem to do that at least by default:
Blackfriday itself does nothing to protect against malicious content. If you are dealing with user-supplied markdown, we recommend running Blackfriday's output through HTML sanitizer such as Bluemonday.
|
|
||
| t, err := template.ParseFiles(filepath.Join(resourceDir, "template.html")) | ||
| if err != nil { | ||
| logrus.WithError(err).Error("Error executing template.") |
There was a problem hiding this comment.
| logrus.WithError(err).Error("Error executing template.") | |
| logrus.WithError(err).Error("Failed to load template.") |
k8s-ci-robot
removed
the
lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: michelle192837, vovakrk The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
4 participants
Summary component is intended for rendering markdown (MD) files.