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

fix: sanitize highlight text #71

Closed

Conversation

takanome-dev
Copy link
Contributor

@takanome-dev takanome-dev commented Aug 21, 2023

Description

This PR fixes the issue with sanitizing the highlighted text. The code has been modified to ensure that the text is properly sanitized, preventing any potential security vulnerabilities. This improvement enhances the overall security of the application.

Generated using OpenSauced.

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

Fixes #66

Mobile & Desktop Screenshots/Recordings

Highlight Before sanitization After
2023-08-21_20-29_1 30-before 30-escape
2023-08-21_20-29 17-before 17-escape

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ docs.opensauced.pizza
  • πŸ• dev.to/opensauced
  • πŸ“• storybook
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

@takanome-dev
Copy link
Contributor Author

I think we should be on the side of escaping everything and have it rendered as and not em. I think people will recognize escaped HTML characters in the highlight text.
originally posted by @brandonroberts in #66 (comment)

Need to figure out how to do this

@brandonroberts
Copy link
Contributor

Usually if the library sanitizes the HTML, it replaces all HTML entities with their equivalent, such as < becoming <

If this one doesn't do that, we should find one that does.

@takanome-dev
Copy link
Contributor Author

takanome-dev commented Aug 22, 2023

it does do it but if the sanitized text is passed to satori, it will treat it as html and it can break the layout (just guessing but I think that makes sense)

<p tw="font-normal text-48px text-light-slate-11 tracking-tight">
${body.length > 108 ? `${body.slice(0, 108)}...` : body}
</p>

We can also just escape the html...

@brandonroberts
Copy link
Contributor

@takanome-dev Ok, try escaping the HTML and see if that works. If so, we can go with that

@takanome-dev takanome-dev marked this pull request as ready for review August 22, 2023 19:31
@takanome-dev
Copy link
Contributor Author

@takanome-dev Ok, try escaping the HTML and see if that works. If so, we can go with that

Done πŸš€

@brandonroberts
Copy link
Contributor

@takanome-dev instead of discarding them, is there no option to sanitize them as-is? So <em>, for example, is still displayed as <em> but not applied as HTML?

@takanome-dev
Copy link
Contributor Author

That's what I have been trying to do. The sanitize-html that I have use can do it, but didn't find how to display html as text with satori. Will dig more into that.

@brandonroberts
Copy link
Contributor

That's what I have been trying to do. The sanitize-html that I have use can do it, but didn't find how to display html as text with satori. Will dig more into that.

Ok. I see more how sanitize-html works, as it will allow certain tags and discard everything else. It may be the best path for now as its a proven solution, but let us know what you find.

@brandonroberts
Copy link
Contributor

@takanome-dev did you find anymore alternatives here?

@takanome-dev
Copy link
Contributor Author

@takanome-dev did you find anymore alternatives here?

@brandonroberts sorry for the late response. Unfortunately, I didn't find any alternative or a way to display the html as a text in the generated og image.

@brandonroberts
Copy link
Contributor

Ok cool. No problem

@nickytonline
Copy link
Member

Closing this for now as there has been no movement since late August. Feel free to reopen @brandonroberts @takanome-dev if work resumes.

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.

Bug: Highlight social images are broken
4 participants