Skip to content

Commit 6ab8104

Browse files
egeekialclaude
andauthored
[newsfeed] add allowBasicHtmlTags option for basic emphasis (MagicMirrorOrg#4176)
**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 (&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. Before screenshot: <img width="980" height="2726" alt="before" src="https://github.com/user-attachments/assets/d1c871e1-21c5-44f9-ae40-da65c2c56f68" /> After screenshot: <img width="980" height="2726" alt="after" src="https://github.com/user-attachments/assets/22d9e86b-221c-408e-a29b-718b0e98f236" /> > 3. 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! --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 4377071 commit 6ab8104

8 files changed

Lines changed: 252 additions & 19 deletions

File tree

defaultmodules/newsfeed/newsfeed.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,8 @@ Module.register("newsfeed", {
3333
prohibitedWords: [],
3434
scrollLength: 500,
3535
logFeedWarnings: false,
36-
dangerouslyDisableAutoEscaping: false
36+
dangerouslyDisableAutoEscaping: false,
37+
allowedBasicHtmlTags: []
3738
},
3839

3940
getUrlPrefix (item) {

defaultmodules/newsfeed/newsfeed.njk

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,13 @@
4545
{% if config.showPublishDate %}{{ item.publishDate }}:{% endif %}
4646
</div>
4747
{% endif %}
48-
<div class="newsfeed-title bright medium light{{ ' no-wrap' if not config.wrapTitle }}">{{ escapeTitle(item.title, item.url, config.dangerouslyDisableAutoEscaping, config.showTitleAsUrl) }}</div>
48+
<div class="newsfeed-title bright medium light{{ ' no-wrap' if not config.wrapTitle }}">{{ escapeTitle(item.title, item.url, config.dangerouslyDisableAutoEscaping or (config.allowedBasicHtmlTags | length), config.showTitleAsUrl) }}</div>
4949
{% if config.showDescription %}
5050
<div class="newsfeed-desc small light{{ ' no-wrap' if not config.wrapDescription }}">
5151
{% if config.truncDescription %}
52-
{{ escapeText(item.description | truncate(config.lengthDescription) , config.dangerouslyDisableAutoEscaping) }}
52+
{{ escapeText(item.description | truncate(config.lengthDescription) , config.dangerouslyDisableAutoEscaping or (config.allowedBasicHtmlTags | length)) }}
5353
{% else %}
54-
{{ escapeText(item.description, config.dangerouslyDisableAutoEscaping) }}
54+
{{ escapeText(item.description, config.dangerouslyDisableAutoEscaping or (config.allowedBasicHtmlTags | length)) }}
5555
{% endif %}
5656
</div>
5757
{% endif %}
@@ -68,13 +68,13 @@
6868
{% if config.showPublishDate %}{{ publishDate }}:{% endif %}
6969
</div>
7070
{% endif %}
71-
<div class="newsfeed-title bright medium light{{ ' no-wrap' if not config.wrapTitle }}">{{ escapeTitle(title, url, config.dangerouslyDisableAutoEscaping, config.showTitleAsUrl) }}</div>
71+
<div class="newsfeed-title bright medium light{{ ' no-wrap' if not config.wrapTitle }}">{{ escapeTitle(title, url, config.dangerouslyDisableAutoEscaping or (config.allowedBasicHtmlTags | length), config.showTitleAsUrl) }}</div>
7272
{% if config.showDescription %}
7373
<div class="newsfeed-desc small light{{ ' no-wrap' if not config.wrapDescription }}">
7474
{% if config.truncDescription %}
75-
{{ escapeText(description | truncate(config.lengthDescription) , config.dangerouslyDisableAutoEscaping) }}
75+
{{ escapeText(description | truncate(config.lengthDescription) , config.dangerouslyDisableAutoEscaping or (config.allowedBasicHtmlTags | length)) }}
7676
{% else %}
77-
{{ escapeText(description, config.dangerouslyDisableAutoEscaping) }}
77+
{{ escapeText(description, config.dangerouslyDisableAutoEscaping or (config.allowedBasicHtmlTags | length)) }}
7878
{% endif %}
7979
</div>
8080
{% endif %}

defaultmodules/newsfeed/newsfeedfetcher.js

Lines changed: 91 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,26 @@ const { htmlToText } = require("html-to-text");
66
const Log = require("logger");
77
const HTTPFetcher = require("#http_fetcher");
88

9+
// The complete set of basic formatting tags users are allowed to opt into via the
10+
// `allowedBasicHtmlTags` config option. These are inline emphasis / line-break tags that
11+
// never carry attributes once sanitized, so they cannot be used for injection. Anything
12+
// requested outside this list is ignored (see the constructor).
13+
const SAFE_HTML_TAGS = ["b", "strong", "i", "em", "u", "br", "code", "s", "sub", "sup"];
14+
15+
// html-to-text formatter that re-emits an allowed inline tag around its content,
16+
// so feeds that send real <em>/<strong> elements keep their emphasis. `br` is a void
17+
// element, so it is emitted as a single self-contained tag with no children/closing tag.
18+
const keepTagFormatter = (elem, walk, builder, formatOptions) => {
19+
const { tagName } = formatOptions;
20+
if (tagName === "br") {
21+
builder.addLiteral("<br>");
22+
return;
23+
}
24+
builder.addLiteral(`<${tagName}>`);
25+
walk(elem.children, builder);
26+
builder.addLiteral(`</${tagName}>`);
27+
};
28+
929
/**
1030
* NewsfeedFetcher - Fetches and parses RSS/Atom feed data
1131
* Uses HTTPFetcher for HTTP handling with intelligent error handling
@@ -20,12 +40,22 @@ class NewsfeedFetcher {
2040
* @param {string} encoding - Encoding of the feed (e.g., 'UTF-8')
2141
* @param {boolean} logFeedWarnings - If true log warnings when there is an error parsing a news article
2242
* @param {boolean} useCorsProxy - If true cors proxy is used for article url's
43+
* @param {string[]} allowedBasicHtmlTags - Basic formatting tags to keep in title and description. Only tags from the safe list are honored; anything else is ignored.
2344
*/
24-
constructor (url, reloadInterval, encoding, logFeedWarnings, useCorsProxy) {
45+
constructor (url, reloadInterval, encoding, logFeedWarnings, useCorsProxy, allowedBasicHtmlTags = []) {
2546
this.url = url;
2647
this.encoding = encoding;
2748
this.logFeedWarnings = logFeedWarnings;
2849
this.useCorsProxy = useCorsProxy;
50+
51+
// Keep only tags from the hardcoded safe list; warn about (and ignore) anything else.
52+
const requestedTags = (Array.isArray(allowedBasicHtmlTags) ? allowedBasicHtmlTags : []).map((tag) => String(tag).trim().toLowerCase());
53+
this.allowedBasicHtmlTags = requestedTags.filter((tag) => SAFE_HTML_TAGS.includes(tag));
54+
const ignoredTags = requestedTags.filter((tag) => !SAFE_HTML_TAGS.includes(tag));
55+
if (ignoredTags.length > 0) {
56+
Log.warn(`Ignoring unsupported allowedBasicHtmlTags [${ignoredTags.join(", ")}] for url ${url}. Allowed tags are: ${SAFE_HTML_TAGS.join(", ")}`);
57+
}
58+
2959
this.items = [];
3060
this.fetchFailedCallback = () => {};
3161
this.itemsReceivedCallback = () => {};
@@ -44,6 +74,48 @@ class NewsfeedFetcher {
4474
this.httpFetcher.on("error", (errorInfo) => this.fetchFailedCallback(this, errorInfo));
4575
}
4676

77+
/**
78+
* Sanitizes a feed string, keeping only the given allowlist of basic
79+
* formatting tags and neutralizing everything else.
80+
*
81+
* The approach is allowlist-only and therefore safe to render unescaped:
82+
* html-to-text first strips all real markup (scripts, links, images, …) and
83+
* decodes entities to text, then EVERYTHING is HTML-escaped and ONLY the exact,
84+
* attribute-free allowlisted tags are restored. No attributes, event handlers,
85+
* or other tags can survive, so arbitrary HTML/script injection is impossible.
86+
* @param {string} html - The raw title or description from the feed.
87+
* @param {string[]} [allowedTags] - Tags to keep. Callers pass an already-validated subset of SAFE_HTML_TAGS.
88+
* @returns {string} Safe HTML containing at most the allowed formatting tags.
89+
*/
90+
static sanitizeBasicHtml (html, allowedTags = []) {
91+
// `br` keeps its default "collapse to a space" behavior unless explicitly allowed.
92+
const keepTagSelectors = allowedTags.map((tagName) => ({ selector: tagName, format: "keepTag", options: { tagName } }));
93+
94+
const text = htmlToText(html, {
95+
wordwrap: false,
96+
formatters: { keepTag: keepTagFormatter },
97+
selectors: [
98+
{ selector: "a", options: { ignoreHref: true, noAnchorUrl: true } },
99+
{ selector: "br", format: "inlineSurround", options: { prefix: " " } },
100+
{ selector: "img", format: "skip" },
101+
...keepTagSelectors
102+
]
103+
});
104+
105+
const escaped = text
106+
.replaceAll("&", "&amp;")
107+
.replaceAll("<", "&lt;")
108+
.replaceAll(">", "&gt;");
109+
110+
if (allowedTags.length === 0) {
111+
return escaped;
112+
}
113+
114+
// Restore only the exact, attribute-free allowed opening/closing tags after escaping.
115+
const restoreAllowedTags = new RegExp(`&lt;(/?(?:${allowedTags.join("|")}))&gt;`, "g");
116+
return escaped.replace(restoreAllowedTags, "<$1>");
117+
}
118+
47119
/**
48120
* Creates a parse error info object
49121
* @param {string} message - Error message
@@ -84,22 +156,30 @@ class NewsfeedFetcher {
84156
const url = item.url || item.link || "";
85157

86158
if (title && pubdate) {
87-
// Convert HTML entities, codes and tag
88-
description = htmlToText(description, {
89-
wordwrap: false,
90-
selectors: [
91-
{ selector: "a", options: { ignoreHref: true, noAnchorUrl: true } },
92-
{ selector: "br", format: "inlineSurround", options: { prefix: " " } },
93-
{ selector: "img", format: "skip" }
94-
]
95-
});
159+
let displayTitle = title;
160+
if (this.allowedBasicHtmlTags.length > 0) {
161+
// Keep the configured basic formatting tags in both fields, strip everything else
162+
description = NewsfeedFetcher.sanitizeBasicHtml(description, this.allowedBasicHtmlTags);
163+
displayTitle = NewsfeedFetcher.sanitizeBasicHtml(title, this.allowedBasicHtmlTags);
164+
} else {
165+
// Convert HTML entities, codes and tag
166+
description = htmlToText(description, {
167+
wordwrap: false,
168+
selectors: [
169+
{ selector: "a", options: { ignoreHref: true, noAnchorUrl: true } },
170+
{ selector: "br", format: "inlineSurround", options: { prefix: " " } },
171+
{ selector: "img", format: "skip" }
172+
]
173+
});
174+
}
96175

97176
this.items.push({
98-
title,
177+
title: displayTitle,
99178
description,
100179
pubdate,
101180
url,
102181
useCorsProxy: this.useCorsProxy,
182+
// Hash on the original title so the dedup identity is stable regardless of allowedBasicHtmlTags
103183
hash: crypto.createHash("sha256").update(`${pubdate} :: ${title} :: ${url}`).digest("hex")
104184
});
105185
} else if (this.logFeedWarnings) {

defaultmodules/newsfeed/node_helper.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ module.exports = NodeHelper.create({
6161
let fetcher;
6262
if (typeof this.fetchers[url] === "undefined") {
6363
Log.log(`Create new newsfetcher for url: ${url} - Interval: ${reloadInterval}`);
64-
fetcher = new NewsfeedFetcher(url, reloadInterval, encoding, config.logFeedWarnings, useCorsProxy);
64+
fetcher = new NewsfeedFetcher(url, reloadInterval, encoding, config.logFeedWarnings, useCorsProxy, config.allowedBasicHtmlTags);
6565

6666
fetcher.onReceive(() => {
6767
this.broadcastFeeds();
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
let config = {
2+
address: "0.0.0.0",
3+
ipWhitelist: [],
4+
timeFormat: 12,
5+
6+
modules: [
7+
{
8+
module: "newsfeed",
9+
position: "bottom_bar",
10+
config: {
11+
feeds: [
12+
{
13+
title: "Formatting Feed",
14+
url: "http://localhost:8080/tests/mocks/newsfeed_basic_html.xml"
15+
}
16+
],
17+
showDescription: true,
18+
truncDescription: false,
19+
allowedBasicHtmlTags: ["b", "strong", "i", "em", "u", "br", "code", "s", "sub", "sup"]
20+
}
21+
}
22+
]
23+
};
24+
25+
/*************** DO NOT EDIT THE LINE BELOW ***************/
26+
if (typeof module !== "undefined") {
27+
module.exports = config;
28+
}

tests/e2e/modules/newsfeed_spec.js

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,32 @@ const runTests = () => {
4545
});
4646
});
4747

48+
describe("Basic HTML tags", () => {
49+
beforeAll(async () => {
50+
await helpers.startApplication("tests/configs/modules/newsfeed/basic_html.js");
51+
await helpers.getDocument();
52+
page = helpers.getPage();
53+
});
54+
55+
it("should render allowlisted formatting tags in title and description", async () => {
56+
await expect(page.locator(".newsfeed .newsfeed-desc")).toBeVisible();
57+
const descHtml = await page.locator(".newsfeed .newsfeed-desc").innerHTML();
58+
expect(descHtml).toContain("<em>");
59+
expect(descHtml).toContain("<strong>");
60+
expect(descHtml).toContain("<u>");
61+
const titleHtml = await page.locator(".newsfeed .newsfeed-title").innerHTML();
62+
expect(titleHtml).toContain("<em>");
63+
});
64+
65+
it("should strip disallowed HTML and not execute injected scripts", async () => {
66+
const descHtml = await page.locator(".newsfeed .newsfeed-desc").innerHTML();
67+
expect(descHtml).not.toContain("<script");
68+
expect(descHtml).not.toContain("onerror");
69+
const xss = await page.evaluate(() => window.__newsfeedXss);
70+
expect(xss).toBeUndefined();
71+
});
72+
});
73+
4874
describe("Invalid configuration", () => {
4975
beforeAll(async () => {
5076
await helpers.startApplication("tests/configs/modules/newsfeed/incorrect_url.js");
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<rss version="2.0">
3+
<channel>
4+
<title>Formatting Feed</title>
5+
<link>http://localhost:8080</link>
6+
<description>Feed used to test the allowBasicHtmlTags option.</description>
7+
<item>
8+
<title>News &lt;em&gt;Flash&lt;/em&gt;</title>
9+
<link>http://localhost:8080/article</link>
10+
<pubDate>Tue, 20 Sep 2016 11:16:08 +0000</pubDate>
11+
<guid isPermaLink="false">http://localhost:8080/?p=1</guid>
12+
<description><![CDATA[<p><em>Italic</em> and <strong>Bold</strong> and <u>Underlined</u> text.</p><script>window.__newsfeedXss = true;</script><img src="x" onerror="window.__newsfeedXss = true">]]></description>
13+
</item>
14+
</channel>
15+
</rss>
Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
const defaults = require("../../../../../js/defaults");
2+
3+
const NewsfeedFetcher = require(`../../../../../${defaults.defaultModulesDir}/newsfeed/newsfeedfetcher`);
4+
5+
// The full safe list users may opt into; most tests run with it enabled.
6+
const ALL_TAGS = ["b", "strong", "i", "em", "u", "br", "code", "s", "sub", "sup"];
7+
const sanitize = (html, allowedTags = ALL_TAGS) => NewsfeedFetcher.sanitizeBasicHtml(html, allowedTags);
8+
9+
describe("NewsfeedFetcher.sanitizeBasicHtml", () => {
10+
it("keeps real basic formatting tags", () => {
11+
expect(sanitize("<b>a</b> <strong>b</strong> <i>c</i> <em>d</em> <u>e</u>"))
12+
.toBe("<b>a</b> <strong>b</strong> <i>c</i> <em>d</em> <u>e</u>");
13+
});
14+
15+
it("keeps the additional safe tags (code, s, sub, sup)", () => {
16+
expect(sanitize("<code>x</code> <s>y</s> <sub>z</sub> <sup>w</sup>"))
17+
.toBe("<code>x</code> <s>y</s> <sub>z</sub> <sup>w</sup>");
18+
});
19+
20+
it("renders entity-encoded formatting tags (e.g. The Atlantic feed)", () => {
21+
// Feeds like theatlantic.com ship emphasis as escaped entities
22+
expect(sanitize("the &lt;em&gt;Atlantic&lt;/em&gt; ocean")).toBe("the <em>Atlantic</em> ocean");
23+
});
24+
25+
it("handles emphasis inside titles regardless of how the parser delivers it", () => {
26+
// The Atlantic uses <em> in titles, e.g. "That's Enough, <em>Euphoria</em>"
27+
const expected = "That’s Enough, <em>Euphoria</em>";
28+
expect(sanitize("That’s Enough, <em>Euphoria</em>")).toBe(expected);
29+
expect(sanitize("That’s Enough, &lt;em&gt;Euphoria&lt;/em&gt;")).toBe(expected);
30+
});
31+
32+
it("strips attributes from allowed tags", () => {
33+
const result = sanitize("<b onclick=\"steal()\" class=\"x\">bold</b>");
34+
expect(result).toBe("<b>bold</b>");
35+
expect(result).not.toContain("onclick");
36+
expect(result).not.toContain("class");
37+
});
38+
39+
it("neutralizes script tags", () => {
40+
expect(sanitize("<script>alert(1)</script>hello")).not.toContain("<script");
41+
// Entity-encoded scripts must stay inert text, never become live markup
42+
const encoded = sanitize("&lt;script&gt;alert(1)&lt;/script&gt;");
43+
expect(encoded).not.toContain("<script");
44+
expect(encoded).toContain("&lt;script&gt;");
45+
});
46+
47+
it("drops images and link hrefs but keeps disallowed-tag text", () => {
48+
const result = sanitize("<img src=\"x\" onerror=\"alert(1)\"><a href=\"https://evil.example\">link</a><h1>title</h1>");
49+
expect(result).not.toContain("onerror");
50+
expect(result).not.toContain("href");
51+
expect(result).not.toContain("<h1>");
52+
expect(result).toContain("link");
53+
expect(result.toLowerCase()).toContain("title");
54+
});
55+
56+
it("escapes bare HTML special characters in plain text", () => {
57+
expect(sanitize("Fish &amp; Chips for &lt; 5")).toBe("Fish &amp; Chips for &lt; 5");
58+
});
59+
60+
it("only keeps tags present in the supplied allowlist", () => {
61+
// Allow just <em>: a safe-but-not-allowed <strong> must become plain text.
62+
const result = sanitize("<em>kept</em> <strong>dropped</strong>", ["em"]);
63+
expect(result).toBe("<em>kept</em> dropped");
64+
expect(result).not.toContain("<strong>");
65+
});
66+
67+
it("escapes everything when the allowlist is empty", () => {
68+
expect(sanitize("<em>hi</em> &amp; <b>bye</b>", [])).toBe("hi &amp; bye");
69+
});
70+
71+
it("renders <br> as a single self-closing tag when allowed", () => {
72+
const result = sanitize("a<br>b", ["br"]);
73+
expect(result).toContain("<br>");
74+
expect(result).not.toContain("<br></br>");
75+
expect(result).not.toContain("&lt;br&gt;");
76+
});
77+
78+
it("collapses <br> to a space when not allowed", () => {
79+
const result = sanitize("a<br>b", ["em"]);
80+
expect(result).not.toContain("<br>");
81+
expect(result).toBe("a b");
82+
});
83+
});

0 commit comments

Comments
 (0)