Skip to content

fix: add output encoding in MMM-CalendarExtMiniMonth.js (CWE-79) - #23

Closed
anupamme wants to merge 2 commits into
MagicMirrorModules:mainfrom
anupamme:fix-repo-mmm-calendarextminimonth-cwe-79-sanitize-event-title
Closed

anupamme wants to merge 2 commits into
MagicMirrorModules:mainfrom
anupamme:fix-repo-mmm-calendarextminimonth-cwe-79-sanitize-event-title

Conversation

@anupamme

@anupamme anupamme commented Oct 2, 2026 •

Copy link
Copy Markdown

Original finding

The automated scan flagged updateContentFromCalendarEvents for storing event.title from untrusted calendar sources (Google Calendar, CalDAV, ICS feeds) without sanitization, and the original version of this PR escaped event.title at ingestion time with a new escapeHtml() helper.

A maintainer asked a fair question in review: is there actually a code path that renders the event title into the DOM? If not, escaping on input isn't the right fix, and it might make more sense to drop the field instead.

Investigation

MMM-CalendarExtMiniMonth.js is the only JS source file in this module. getDom() and drawSlot() are the only places that build DOM nodes, and the only three innerHTML writes are:

  • header.innerHTML = formatPattern(new Date(), this.config.titleFormat, locale) (month header)
  • cell.innerHTML = formatPattern(d, this.config.weekdayFormat, locale) (weekday initials)
  • cell.innerHTML = formatPattern(date, this.config.dateFormat, locale) (day numbers)

All three are driven by Date objects plus a small fixed config enum ('MMMM', 'D', 'Do', 'dd', …) via formatPattern() — never by event.title. event.title itself is used in exactly one other place: the pre-existing required-field filter (event.name && event.title && Number.isFinite(event.startDate) && Number.isFinite(event.endDate)), which just drops events missing a title. That filter predates this PR.

Conclusion: there is no reachable DOM sink for event.title in this module today. Escaping it at ingestion was solving a problem that doesn't currently exist, was applied inconsistently (the CALEXT2 ingestion path never went through escapeHtml either), and actively corrupted the stored data for a value that's never displayed (e.g. Research & Development became Research & Development in memory).

Updated fix

  • Reverted the escapeHtml() helper and its call site — title is stored raw again, exactly as on main.
  • Kept the title field on the internal event object, since it's still required by the pre-existing filter rule above; removing it would change unrelated filtering behavior.
  • Added a left-behind comment at the storage site pointing future readers at the regression test instead of re-adding ad-hoc escaping if a rendering feature is ever added.
  • Added test/event-title-no-dom-sink.test.js (Node's built-in node:test, no new dependencies) that feeds normal and malicious titles (<script>alert(1)</script>, <img src=x onerror=alert(1)>, Research & Development, Tom's "Meeting", empty/missing) through updateContentFromCalendarEvents() + getDom(), and asserts: (a) titles are stored verbatim, and (b) they never appear in the serialized rendered DOM output. Verified this test actually fails if a title-rendering sink is reintroduced (temporarily added + slot.events[0].title to a cell.innerHTML assignment, confirmed the test caught it, then reverted).
  • Wired the test into npm test (node --run lint && node --test), which CI already runs.

Verification

  • npm test passes locally (lint + 6 new test cases).
  • Confirmed the test fails when a title-rendering sink is reintroduced, and passes once removed — the test has real teeth, not just happy-path coverage.
  • Diff against main for MMM-CalendarExtMiniMonth.js now only adds a comment; the title: event.title, line is byte-for-byte what's on main.

Automated security fix by OrbisAI Security

Automated security fix generated by OrbisAI Security
@KristjanESPERANTO

Copy link
Copy Markdown
Member

@anupamme Thanks for the PR! I have a question: I couldn’t find any code path that renders the event title in the DOM. Would it make more sense to remove the title from the internal event object, since it doesn’t seem to be needed, rather than escaping it on input? If I’ve missed a place/an option where it is rendered, could you point me to the specific DOM sink?

… guarantee

event.title is never inserted into the DOM anywhere in this module (getDom/drawSlot
only write innerHTML from Date/config-derived strings via formatPattern, never from
event data), so escaping it at ingestion time fixed nothing live and corrupted the
stored title (e.g. "&" became "&amp;"). Revert the escapeHtml() call, keep the title
field (still required by the pre-existing name/title/date filter), and add a test
that proves no DOM sink exists today and will fail if one is ever added unsafely.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@anupamme

Copy link
Copy Markdown
Author

Thanks for pushing on this — you're right, I couldn't find a sink either after re-checking.

The only innerHTML writes in MMM-CalendarExtMiniMonth.js are in getDom()/drawSlot() (month header, weekday initials, day numbers), and all three are built from Date objects plus a fixed config format enum via formatPattern() — never from event.title. event.title itself is only used for the pre-existing required-field filter (event.name && event.title && Number.isFinite(event.startDate) && Number.isFinite(event.endDate)), which predates this PR.

So I've updated the PR: kept the title field (still needed for that filter), but removed the escapeHtml() call — it wasn't fixing anything live and was corrupting the stored title (e.g. & → &amp;). Added a regression test (test/event-title-no-dom-sink.test.js, now run via npm test/CI) that feeds <script>/onerror payloads through and asserts they never show up in the rendered DOM, so this stays caught if a title-rendering feature is ever added later without proper escaping/textContent. I verified the test actually catches a reintroduced sink before relying on it.

PR description updated with the full writeup. Appreciate you catching this.

@KristjanESPERANTO

Copy link
Copy Markdown
Member

Thanks for working on this. It prompted me to clean up the code and remove the unused event-title handling. I’m closing this PR because the proposed change is no longer needed.

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