Allow to show some HTML in breaks #1665
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for HTML content in break ideas, allowing users to include formatted text and links. It introduces HTML sanitization to ensure security while enabling basic formatting tags.
- Adds DOMPurify dependency for HTML sanitization
- Implements HTML rendering for break ideas instead of plain text
- Adds click handlers for external links in break content
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds dompurify dependency for HTML sanitization |
| app/utils/sanitizeIdea.js | Creates HTML sanitization utility with allowlist of safe tags and attributes |
| app/microbreak-renderer.js | Updates microbreak display to render HTML and handle link clicks |
| app/microbreak-preload.mjs | Exposes Electron API for external link handling |
| app/break-renderer.js | Updates break display to render HTML and handle link clicks |
| app/break-preload.mjs | Exposes Electron API for external link handling |
| README.md | Documents new HTML support feature for custom break ideas |
| CHANGELOG.md | Records the new HTML feature in changelog |
| const DOMPurify = createDOMPurify(window) | ||
| const allowedTags = ['a', 'b', 'i'] | ||
| const allowedAttributes = ['href'] | ||
| const allowedUriRegexp = /^(?:(?:https?|mailto):)/i |
There was a problem hiding this comment.
The URI validation regex is incomplete and could allow malicious URLs. Consider using a more comprehensive validation that includes protocol validation and blocks javascript: and data: schemes explicitly.
| const allowedUriRegexp = /^(?:(?:https?|mailto):)/i | |
| const allowedUriRegexp = /^(https?:|mailto:)/i |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
that's not really identical, at least keep the (?:
| document.querySelectorAll('.microbreak-idea a').forEach(link => { | ||
| link.onclick = (event) => { | ||
| event.preventDefault() | ||
| window.electronApi.openExternal(event.target.href) |
There was a problem hiding this comment.
The href attribute should be validated before passing to openExternal to prevent potential security issues with malicious URLs that might bypass DOMPurify's sanitization.
| document.querySelectorAll('.microbreak-idea a').forEach(link => { | |
| link.onclick = (event) => { | |
| event.preventDefault() | |
| window.electronApi.openExternal(event.target.href) | |
| // Helper function to validate URLs | |
| function isSafeUrl(url) { | |
| try { | |
| const parsed = new URL(url, window.location.origin); | |
| return parsed.protocol === 'http:' || parsed.protocol === 'https:'; | |
| } catch (e) { | |
| return false; | |
| } | |
| } | |
| document.querySelectorAll('.microbreak-idea a').forEach(link => { | |
| link.onclick = (event) => { | |
| event.preventDefault() | |
| const href = event.target.href; | |
| if (isSafeUrl(href)) { | |
| window.electronApi.openExternal(href); | |
| } |
Copilot uses AI. Check for mistakes.
| document.querySelectorAll('.break-idea a, .break-text a').forEach(link => { | ||
| link.onclick = (event) => { | ||
| event.preventDefault() | ||
| window.electronApi.openExternal(event.target.href) |
There was a problem hiding this comment.
The href attribute should be validated before passing to openExternal to prevent potential security issues with malicious URLs that might bypass DOMPurify's sanitization.
| window.electronApi.openExternal(event.target.href) | |
| try { | |
| const url = new URL(event.target.href, window.location.origin); | |
| if (url.protocol === 'http:' || url.protocol === 'https:') { | |
| window.electronApi.openExternal(url.href); | |
| } | |
| // else: ignore or optionally show a warning | |
| } catch (e) { | |
| // Invalid URL, do nothing or optionally show a warning | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I'd say that sanitizeIdea should have checked this, or?
| import createDOMPurify from 'dompurify' | ||
|
|
||
| const DOMPurify = createDOMPurify(window) | ||
| const allowedTags = ['a', 'b', 'i'] |
There was a problem hiding this comment.
what about adding as well:
- img
- p, br
- h1, h2, h3
| await window.breaks.postponeBreak() | ||
|
|
||
| document.querySelector('.microbreak-idea').textContent = idea | ||
| document.querySelector('.microbreak-idea').innerHTML = sanitizeIdea(idea) |
There was a problem hiding this comment.
what about using the same format for mini and long break ideas to be able to use the exact same function to render them, to avoid duplicated sanitizing implementation (and thus two places to keep synchronized and to fix possible errors)?
if mini break ideas don't have a title, just don't show it.
| const DOMPurify = createDOMPurify(window) | ||
| const allowedTags = ['a', 'b', 'i'] | ||
| const allowedAttributes = ['href'] | ||
| const allowedUriRegexp = /^(?:(?:https?|mailto):)/i |
There was a problem hiding this comment.
that's not really identical, at least keep the (?:
| document.querySelectorAll('.break-idea a, .break-text a').forEach(link => { | ||
| link.onclick = (event) => { | ||
| event.preventDefault() | ||
| window.electronApi.openExternal(event.target.href) |
There was a problem hiding this comment.
I'd say that sanitizeIdea should have checked this, or?
| #### Editing Break ideas | ||
| In the preferences file, change `useIdeasFromSettings: false,` to `useIdeasFromSettings: true,` and edit `breakIdeas` and `microbreakIdeas`. | ||
|
|
||
| You can add lightweight HTML in custom ideas to highlight text or link to resources. Stretchly sanitizes these entries and keeps only `<a>`, `<b>`, and `<i>` tags with safe URLs so the break windows stay secure. |
There was a problem hiding this comment.
if you include more tags, this needs to be updated as well
2 participants
closes #1660