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

render html/markdown for new comment notification email #3255

Merged

Conversation

kimsible
Copy link
Contributor

@kimsible kimsible commented Nov 5, 2020

Fix #3254

@rigelk rigelk self-assigned this Nov 6, 2020
Copy link
Collaborator

@rigelk rigelk left a comment

Choose a reason for hiding this comment

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

hi @kimsible,

In its current state, this PR is using marked while we use markdown-it for the client render, c.f. https://github.com/Chocobozzz/PeerTube/blob/develop/client/src/app/core/renderer/markdown.service.ts. Could you align with our client service and make use of markdown-it in the server too?

As for the way you render HTML, please note we make use of sanitize-html to provide security as per scenario 2 of https://github.com/markdown-it/markdown-it/blob/master/docs/security.md. Could you align in that matter too?

@kimsible
Copy link
Contributor Author

kimsible commented Nov 6, 2020

Hi @rigelk,

In its current state, this PR is using marked while we use markdown-it for the client render, c.f. https://github.com/Chocobozzz/PeerTube/blob/develop/client/src/app/core/renderer/markdown.service.ts. Could you align with our client service and make use of markdown-it in the server too?

I hesitated because marked-man et marked are already used by the CLI. But you're right ! it's better to use the same libs in the app server - client.

As for the way you render HTML, please note we make use of sanitize-html to provide security as per scenario 2 of https://github.com/markdown-it/markdown-it/blob/master/docs/security.md. Could you align in that matter too?

Thanks I forgot that point !

@kimsible
Copy link
Contributor Author

kimsible commented Nov 6, 2020

@rigelk I've checked the code and I have 2 notices :

  • On client side, only markdown emphasis (bold and italic) and lists are not converted to HTML to the database, everything else is HTML, so marked or markdown-it are enough for that, marked is lighter and already a server-side dependency, maybe it is the right choice ? ;
  • On client side, we also sanitize the html, so the text of the comment in the database is already sanitized, do we really need to sanitize it again even if it's come from the database ?

@rigelk
Copy link
Collaborator

rigelk commented Nov 6, 2020

@kimsible please stick with markdown-it. marked is lighter but that doesn't matter when it comes to server-side dependencies, and I prefer consistency with the client part. I don't know why we use marked but that isn't right.

On client side, we also sanitize the html, so the text of the comment in the database is already sanitized, do we really need to sanitize it again even if it's come from the database ?

We sanitize the display, not the storage. Anyway, applying the sanitization is not a costly extra step 😉

@kimsible kimsible force-pushed the fix/render-comment-mail-notification branch from d688ef0 to 22f2f72 Compare November 6, 2020 20:42
@kimsible kimsible requested a review from rigelk November 6, 2020 20:42
@kimsible kimsible force-pushed the fix/render-comment-mail-notification branch from 22f2f72 to 88f440b Compare November 6, 2020 21:24
@kimsible kimsible force-pushed the fix/render-comment-mail-notification branch from 88f440b to 5203c30 Compare November 6, 2020 21:57
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@rigelk rigelk changed the title Render html / markdown comment in notification email render html/markdown for new comment notification email Nov 7, 2020
@rigelk rigelk merged commit 98b9464 into Chocobozzz:develop Nov 7, 2020
@rigelk
Copy link
Collaborator

rigelk commented Nov 7, 2020

@kimsible thanks a bunch :)

@kimsible
Copy link
Contributor Author

kimsible commented Nov 7, 2020

@rigelk thanks for your help !

fflorent pushed a commit to fflorent/PeerTube that referenced this pull request Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No render for comment in notification e-mail
2 participants