Skip to content

[newsfeed] add allowBasicHtmlTags option for basic emphasis#4176

Open
egeekial wants to merge 1 commit into
MagicMirrorOrg:developfrom
egeekial:feat/newsfeed-basic-html
Open

[newsfeed] add allowBasicHtmlTags option for basic emphasis#4176
egeekial wants to merge 1 commit into
MagicMirrorOrg:developfrom
egeekial:feat/newsfeed-basic-html

Conversation

@egeekial
Copy link
Copy Markdown

@egeekial egeekial commented Jun 3, 2026

Please make sure that you have followed these 3 rules before submitting your Pull Request:

  1. Base your pull requests against the develop branch.
    Done.
  2. Include these infos in the description:
  • Does the pull request solve a related issue?
    No
  • If so, can you reference the issue like this Fixes #<issue_number>?
  • What does the pull request accomplish? Use a list if needed.
  • If it includes major visual changes please add screenshots.

Render a strict allowlist of basic formatting tags (b, strong, i, em, u) in news titles and descriptions, while neutralizing all other HTML.

Feeds such as The Atlantic encode emphasis as entities (<em>), which html-to-text decoded to a literal string that the template then auto-escaped, so the raw tag was shown on screen. The new opt-in allowBasicHtmlTags option (default false) sanitizes both fields by escaping everything and restoring only the exact, attribute-free allowlisted tags, so the result is safe to render and arbitrary HTML/script injection is impossible.

Adds unit tests for the sanitizer and an e2e test covering rendering and an injection attempt.

Before screenshot: before
After screenshot: after

  1. Please run node --run lint:prettier before submitting so that
    style issues are fixed.
    Done

Note: Sometimes the development moves very fast. It is highly
recommended that you update your branch of develop before creating a
pull request to send us your changes. This makes everyone's lives
easier (including yours) and helps us out on the development team.

Thanks again and have a nice day!

Render a strict allowlist of basic formatting tags (b, strong, i, em, u)
in news titles and descriptions, while neutralizing all other HTML.

Feeds such as The Atlantic encode emphasis as entities (&lt;em&gt;), which
html-to-text decoded to a literal <em> string that the template then
auto-escaped, so the raw tag was shown on screen. The new opt-in
allowBasicHtmlTags option (default false) sanitizes both fields by
escaping everything and restoring only the exact, attribute-free
allowlisted tags, so the result is safe to render and arbitrary
HTML/script injection is impossible.

Adds unit tests for the sanitizer and an e2e test covering rendering and
an injection attempt.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@sdetweil
Copy link
Copy Markdown
Collaborator

sdetweil commented Jun 3, 2026

do you ever envision the user wanting/needing to adjust the list of tags

in newfeedfetcher.js

const ALLOWED_TAGS = ["b", "strong", "i", "em", "u"];

if this is approved, will you also create a matching PR for the documentation repo?

@egeekial
Copy link
Copy Markdown
Author

egeekial commented Jun 4, 2026

do you ever envision the user wanting/needing to adjust the list of tags

in newfeedfetcher.js

const ALLOWED_TAGS = ["b", "strong", "i", "em", "u"];

My intent was to keep this limited to basic formatting and make the feature something users can simply turn on or off. I could imagine a user wanting finer control, such as allowing italics but not bold or underline, but I suspect most users would not want or need that level of granularity.

If you think that configurability would be beneficial, I would prefer exposing it as a subset of the existing safe list rather than allowing users to add arbitrary tags. Letting users add new tags seems more likely to create rendering issues or introduce an injection risk.

if this is approved, will you also create a matching PR for the documentation repo?

Yes, I can do that.

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.

2 participants