Skip to content

Commit df8a882

Browse files
KristjanESPERANTOKristjanESPERANTO
authored
fix(newsfeed): fix full article view and add framing check (#4039)
I was playing around with the newsfeed notification system (`ARTICLE_MORE_DETAILS`, `ARTICLE_TOGGLE_FULL`, …) and discovered some issues with the full article view: The iframe was loading the CORS proxy URL instead of the actual article URL, which could cause blank screens depending on the feed. Also, many news sites block iframes entirely (`X-Frame-Options: DENY`) and the user got no feedback at all — just an empty page. On top of that, scrolling used `window.scrollTo()` which moved the entire MagicMirror page instead of just the article. This PR cleans that up: - Use the raw article URL for the iframe (CORS proxy is only needed for server-side feed fetching) - Check `X-Frame-Options` / `Content-Security-Policy` headers server-side before showing the iframe — if the site blocks it, show a brief "Article cannot be displayed here." message and return to normal view - Show the iframe as a fixed full-screen overlay so other modules aren't affected, scroll via `container.scrollTop` - Keep the progressive disclosure behavior for `ARTICLE_MORE_DETAILS` (title → description → iframe → scroll) - Delete `fullarticle.njk`, replace with `getDom()` override - Fix `ARTICLE_INFO_RESPONSE` returning proxy URL instead of real URL - A few smaller fixes (negative scroll, null guard) - Add `NEWSFEED_ARTICLE_UNAVAILABLE` translation to all 47 language files - Add e2e tests for the notification handlers (`ARTICLE_NEXT`, `ARTICLE_PREVIOUS`, `ARTICLE_INFO_REQUEST`, `ARTICLE_LESS_DETAILS`) ## What this means for users - The full article view now works reliably across different feeds - If a news site blocks iframes, the user sees a brief message instead of a blank screen - Additional e2e tests make the module more robust and less likely to break silently in future MagicMirror versions
1 parent 729f7f0 commit df8a882

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

53 files changed

+349
-41
lines changed

‎defaultmodules/newsfeed/fullarticle.njk‎

Lines changed: 0 additions & 3 deletions
This file was deleted.

‎defaultmodules/newsfeed/newsfeed.css‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,24 @@
1-
iframe.newsfeed-fullarticle {
1+
.newsfeed-fullarticle-container {
2+
position: fixed;
23
width: 100vw;
3-
4-
/* very large height value to allow scrolling */
5-
height: 3000px;
4+
height: 100vh;
65
top: 0;
76
left: 0;
7+
overflow-y: auto;
8+
scrollbar-width: none;
9+
z-index: 1000;
10+
background: black;
11+
}
12+
13+
.newsfeed-fullarticle-container::-webkit-scrollbar {
14+
display: none;
15+
}
16+
17+
iframe.newsfeed-fullarticle {
18+
display: block;
19+
width: 100%;
20+
height: 5000px;
821
border: none;
9-
z-index: 1;
1022
}
1123

1224
.region.bottom.bar.newsfeed-fullarticle {

‎defaultmodules/newsfeed/newsfeed.js‎

Lines changed: 92 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,10 @@ Module.register("newsfeed", {
7474
this.error = null;
7575
this.activeItem = 0;
7676
this.scrollPosition = 0;
77+
this.articleIframe = null;
78+
this.articleContainer = null;
79+
this.articleFrameCheckPending = false;
80+
this.articleUnavailable = false;
7781

7882
this.registerFeeds();
7983

@@ -97,15 +101,60 @@ Module.register("newsfeed", {
97101
} else if (notification === "NEWSFEED_ERROR") {
98102
this.error = this.translate(payload.error_type);
99103
this.scheduleUpdateInterval();
104+
} else if (notification === "ARTICLE_URL_STATUS") {
105+
if (this.config.showFullArticle) {
106+
this.articleFrameCheckPending = false;
107+
this.articleUnavailable = !payload.canFrame;
108+
if (!this.articleUnavailable) {
109+
// Article can be framed — now shift the bottom bar to allow scrolling
110+
document.getElementsByClassName("region bottom bar")[0].classList.add("newsfeed-fullarticle");
111+
}
112+
this.updateDom(100);
113+
if (this.articleUnavailable) {
114+
// Briefly show the unavailable message, then return to normal newsfeed view
115+
setTimeout(() => {
116+
this.resetDescrOrFullArticleAndTimer();
117+
this.updateDom(500);
118+
}, 3000);
119+
}
120+
}
121+
}
122+
},
123+
124+
//Override getDom to handle the full article case with error handling
125+
getDom () {
126+
if (this.config.showFullArticle) {
127+
this.activeItemHash = this.newsItems[this.activeItem]?.hash;
128+
const wrapper = document.createElement("div");
129+
if (this.articleFrameCheckPending) {
130+
// Still waiting for the server-side framing check
131+
wrapper.innerHTML = `<div class="small dimmed">${this.translate("LOADING")}</div>`;
132+
} else if (this.articleUnavailable) {
133+
wrapper.innerHTML = `<div class="small dimmed">${this.translate("NEWSFEED_ARTICLE_UNAVAILABLE")}</div>`;
134+
} else {
135+
const container = document.createElement("div");
136+
container.className = "newsfeed-fullarticle-container";
137+
container.scrollTop = this.scrollPosition;
138+
const iframe = document.createElement("iframe");
139+
iframe.className = "newsfeed-fullarticle";
140+
// Always use the direct article URL — the CORS proxy is for server-side
141+
// RSS feed fetching, not for browser iframes.
142+
const item = this.newsItems[this.activeItem];
143+
iframe.src = item ? (typeof item.url === "string" ? item.url : item.url.href) : "";
144+
this.articleIframe = iframe;
145+
this.articleContainer = container;
146+
container.appendChild(iframe);
147+
wrapper.appendChild(container);
148+
}
149+
return Promise.resolve(wrapper);
100150
}
151+
return this._super();
101152
},
102153

103154
//Override fetching of template name
104155
getTemplate () {
105156
if (this.config.feedUrl) {
106157
return "oldconfig.njk";
107-
} else if (this.config.showFullArticle) {
108-
return "fullarticle.njk";
109158
}
110159
return "newsfeed.njk";
111160
},
@@ -116,13 +165,6 @@ Module.register("newsfeed", {
116165
this.activeItem = 0;
117166
}
118167
this.activeItemCount = this.newsItems.length;
119-
// this.config.showFullArticle is a run-time configuration, triggered by optional notifications
120-
if (this.config.showFullArticle) {
121-
this.activeItemHash = this.newsItems[this.activeItem]?.hash;
122-
return {
123-
url: this.getActiveItemURL()
124-
};
125-
}
126168
if (this.error) {
127169
this.activeItemHash = undefined;
128170
return {
@@ -358,6 +400,10 @@ Module.register("newsfeed", {
358400
this.isShowingDescription = this.config.showDescription;
359401
this.config.showFullArticle = false;
360402
this.scrollPosition = 0;
403+
this.articleIframe = null;
404+
this.articleContainer = null;
405+
this.articleFrameCheckPending = false;
406+
this.articleUnavailable = false;
361407
// reset bottom bar alignment
362408
document.getElementsByClassName("region bottom bar")[0].classList.remove("newsfeed-fullarticle");
363409
if (!this.timer) {
@@ -386,23 +432,26 @@ Module.register("newsfeed", {
386432
Log.debug(`[newsfeed] going from article #${before} to #${this.activeItem} (of ${this.newsItems.length})`);
387433
this.updateDom(100);
388434
}
389-
// if "more details" is received the first time: show article summary, on second time show full article
390435
else if (notification === "ARTICLE_MORE_DETAILS") {
391-
// full article is already showing, so scrolling down
392436
if (this.config.showFullArticle === true) {
437+
// iframe already showing — scroll down
393438
this.scrollPosition += this.config.scrollLength;
394-
window.scrollTo(0, this.scrollPosition);
395-
Log.debug("[newsfeed] scrolling down");
396-
Log.debug(`[newsfeed] ARTICLE_MORE_DETAILS, scroll position: ${this.config.scrollLength}`);
397-
} else {
439+
if (this.articleContainer) this.articleContainer.scrollTop = this.scrollPosition;
440+
Log.debug(`[newsfeed] scrolling down, offset: ${this.scrollPosition}`);
441+
} else if (this.isShowingDescription) {
442+
// description visible — step up to full article
398443
this.showFullArticle();
444+
} else {
445+
// only title visible — show description first
446+
this.isShowingDescription = true;
447+
Log.debug("[newsfeed] showing article description");
448+
this.updateDom(100);
399449
}
400450
} else if (notification === "ARTICLE_SCROLL_UP") {
401451
if (this.config.showFullArticle === true) {
402-
this.scrollPosition -= this.config.scrollLength;
403-
window.scrollTo(0, this.scrollPosition);
404-
Log.debug("[newsfeed] scrolling up");
405-
Log.debug(`[newsfeed] ARTICLE_SCROLL_UP, scroll position: ${this.config.scrollLength}`);
452+
this.scrollPosition = Math.max(0, this.scrollPosition - this.config.scrollLength);
453+
if (this.articleContainer) this.articleContainer.scrollTop = this.scrollPosition;
454+
Log.debug(`[newsfeed] scrolling up, offset: ${this.scrollPosition}`);
406455
}
407456
} else if (notification === "ARTICLE_LESS_DETAILS") {
408457
this.resetDescrOrFullArticleAndTimer();
@@ -416,26 +465,37 @@ Module.register("newsfeed", {
416465
this.showFullArticle();
417466
}
418467
} else if (notification === "ARTICLE_INFO_REQUEST") {
419-
this.sendNotification("ARTICLE_INFO_RESPONSE", {
420-
title: this.newsItems[this.activeItem].title,
421-
source: this.newsItems[this.activeItem].sourceTitle,
422-
date: this.newsItems[this.activeItem].pubdate,
423-
desc: this.newsItems[this.activeItem].description,
424-
url: this.getActiveItemURL()
425-
});
468+
const infoItem = this.newsItems[this.activeItem];
469+
if (infoItem) {
470+
this.sendNotification("ARTICLE_INFO_RESPONSE", {
471+
title: infoItem.title,
472+
source: infoItem.sourceTitle,
473+
date: infoItem.pubdate,
474+
desc: infoItem.description,
475+
url: typeof infoItem.url === "string" ? infoItem.url : infoItem.url.href
476+
});
477+
}
426478
}
427479
},
428480

429481
showFullArticle () {
430-
this.isShowingDescription = !this.isShowingDescription;
431-
this.config.showFullArticle = !this.isShowingDescription;
432-
// make bottom bar align to top to allow scrolling
433-
if (this.config.showFullArticle === true) {
434-
document.getElementsByClassName("region bottom bar")[0].classList.add("newsfeed-fullarticle");
482+
const item = this.newsItems[this.activeItem];
483+
const hasUrl = item && item.url && (typeof item.url === "string" ? item.url : item.url.href);
484+
if (!hasUrl) {
485+
Log.debug("[newsfeed] no article URL available, skipping full article view");
486+
return;
435487
}
488+
this.isShowingDescription = false;
489+
this.config.showFullArticle = true;
490+
// Check server-side whether the article URL allows framing.
491+
// The bottom bar CSS class is only added once we know the iframe will be shown.
492+
this.articleFrameCheckPending = true;
493+
this.articleUnavailable = false;
494+
const rawUrl = typeof item.url === "string" ? item.url : item.url.href;
495+
this.sendSocketNotification("CHECK_ARTICLE_URL", { url: rawUrl });
436496
clearInterval(this.timer);
437497
this.timer = null;
438-
Log.debug(`[newsfeed] showing ${this.isShowingDescription ? "article description" : "full article"}`);
498+
Log.debug("[newsfeed] showing full article");
439499
this.updateDom(100);
440500
}
441501
});

‎defaultmodules/newsfeed/node_helper.js‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,28 @@ module.exports = NodeHelper.create({
1313
socketNotificationReceived (notification, payload) {
1414
if (notification === "ADD_FEED") {
1515
this.createFetcher(payload.feed, payload.config);
16+
} else if (notification === "CHECK_ARTICLE_URL") {
17+
this.checkArticleUrl(payload.url);
18+
}
19+
},
20+
21+
/**
22+
* Checks whether a URL can be displayed in an iframe by inspecting
23+
* X-Frame-Options and Content-Security-Policy headers server-side.
24+
* @param {string} url The article URL to check.
25+
*/
26+
async checkArticleUrl (url) {
27+
try {
28+
const response = await fetch(url, { method: "HEAD" });
29+
const xfo = response.headers.get("x-frame-options");
30+
const csp = response.headers.get("content-security-policy");
31+
// sameorigin also blocks since the article is on a different origin than MM
32+
const blockedByXFO = xfo && ["deny", "sameorigin"].includes(xfo.toLowerCase().trim());
33+
const blockedByCSP = csp && (/frame-ancestors\s+['"]?none['"]?/).test(csp);
34+
this.sendSocketNotification("ARTICLE_URL_STATUS", { url, canFrame: !blockedByXFO && !blockedByCSP });
35+
} catch {
36+
// Network error or HEAD not supported — let the browser try the iframe anyway
37+
this.sendSocketNotification("ARTICLE_URL_STATUS", { url, canFrame: true });
1638
}
1739
},
1840

‎tests/configs/modules/newsfeed/notifications.js‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
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: "Rodrigo Ramirez Blog",
14+
url: "http://localhost:8080/tests/mocks/newsfeed_test.xml"
15+
}
16+
],
17+
updateInterval: 3600 * 1000 // 1 hour - prevent auto-rotation during tests
18+
}
19+
}
20+
]
21+
};
22+
23+
/*************** DO NOT EDIT THE LINE BELOW ***************/
24+
if (typeof module !== "undefined") {
25+
module.exports = config;
26+
}

0 commit comments

Comments
 (0)