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

Changed markdown parser to as markdown didnt match gitlab. #721

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

Conversation

Mikkelhost
Copy link
Member

@Mikkelhost Mikkelhost commented Jun 9, 2022

Changed markdown renderer, apparently old renderer was not CommonMark compliant. So the mark down parsed from readme on gitlab did not look the same in the webclient.

@Mikkelhost Mikkelhost changed the title Changed markdown parser to as markdown didnt match gitlab. Apperently… Changed markdown parser to as markdown didnt match gitlab. Jun 9, 2022
Comment on lines +52 to +59
var buf bytes.Buffer
renderer := goldmark.New(
goldmark.WithRendererOptions(html.WithUnsafe()),
)
if err := renderer.Convert(md, &buf); err != nil {
log.Error().Msgf("Error converting to commonmark: %s", err)
}
unsafeHtml := buf.Bytes()
Copy link
Member

Choose a reason for hiding this comment

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

let's make this part as function and just call it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha im so stupid, i'll do that as soon i get time! :D

goldmark.WithRendererOptions(html.WithUnsafe()),
)
if err := renderer.Convert(md, &buf); err != nil {
log.Error().Msgf("Error converting to commonmark: %s", err)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
log.Error().Msgf("Error converting to commonmark: %s", err)
log.Error().Err(err).Msg("converting to markdown")

Use the fields of the log package, might be nice if decided to do central logging for the distributed version later on as it makes it nicer to filter logs

goldmark.WithRendererOptions(html.WithUnsafe()),
)
if err := renderer.Convert(md, &buf); err != nil {
log.Error().Msgf("Error converting to commonmark: %s", err)
Copy link
Member

Choose a reason for hiding this comment

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

See my other comments

goldmark.WithRendererOptions(html.WithUnsafe()),
)
if err := renderer.Convert(md, &buf); err != nil {
log.Error().Msgf("Error converting to commonmark: %s", err)
Copy link
Member

Choose a reason for hiding this comment

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

Again log and errors shouldn't start with error

@eyJhb
Copy link
Member

eyJhb commented Aug 9, 2022

Reconsider if it makes sense to use bluemonday at all, since you're already taking "trusted" input, ie. not user specified input.
Doesn't seem to make sense to use Goldmark with unsafe HTML, and then try to sanitize it afterwards.

@Mikkelhost
Copy link
Member Author

Reconsider if it makes sense to use bluemonday at all, since you're already taking "trusted" input, ie. not user specified input. Doesn't seem to make sense to use Goldmark with unsafe HTML, and then try to sanitize it afterwards.

I think when i created this i pretty much went from a zero trust perspective. And i think it is a good idea to keep it as a zero trust. For the goldmark with unsafe html, i will take a look, i havn't really done anything else thank making it work :)

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