fix: prevent timer leaks and add HTML sanitization layer#206
Open
msrofficial wants to merge 3 commits into
Open
fix: prevent timer leaks and add HTML sanitization layer#206msrofficial wants to merge 3 commits into
msrofficial wants to merge 3 commits into
Conversation
… SkillsHub - DirectoryHub.vue: Added onBeforeUnmount to clear toastTimer and composioSearchTimer on unmount - SkillsHub.vue: Added onBeforeUnmount to clear toastTimer on unmount Prevents timer leaks when components are unmounted before timers fire.
- Create src/utils/sanitizeHtml.ts: lightweight native DOMParser-based HTML sanitizer that strips script, iframe, object elements and event-handler attributes - Create src/components/SafeHtml.vue: reusable wrapper component - ThreadConversation.vue: add sanitizeHtml() to renderMarkdownBlocksAsHtml(), renderListItemContentAsHtml(), and renderCachedHighlightedCodeAsHtml() - SkillDetailModal.vue: add sanitizeHtml() to renderedReadme computed
…ow in script section)
Contributor
📝 WalkthroughWalkthroughAdds a DOMParser-based HTML sanitizer and SafeHtml wrapper, routes several render paths through the sanitizer, and clears pending timers in two content components during unmount. ChangesHTML sanitization path
Unmount timer cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoFix timer leaks and sanitize all v-html HTML rendering paths Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. sanitizeHtml missing manual tests
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
3759
to
3766
| markdownHtmlCache.set(cacheKey, cached) | ||
| return cached.html | ||
| } | ||
| const html = parseMessageBlocks(text) | ||
| const rawHtml = parseMessageBlocks(text) | ||
| .map((block) => renderMessageBlockAsHtml(block)) | ||
| .join('') | ||
| const html = sanitizeHtml(rawHtml) | ||
| return setBoundedCacheEntry( |
There was a problem hiding this comment.
1. sanitizehtml missing manual tests 📘 Rule violation ⚙ Maintainability
This PR changes user-visible behavior by sanitizing multiple v-html markdown/HTML rendering entry points and updates lifecycle behavior by clearing toastTimer/composioSearchTimer on unmount to prevent timer leaks, but it does not include corresponding updates to manual test documentation under tests/<domain>/. Without updated verification steps and expectations, these changes are harder to validate and navigation/unmount regressions may go unnoticed.
Agent Prompt
## Issue description
The PR introduces behavior changes (sanitizing HTML/markdown rendering via `v-html` entry points and clearing timers on component unmount to prevent leaks), but it does not add or update the relevant manual test documentation under `tests/<domain>/`, making these changes harder to verify and more likely to regress unnoticed.
## Issue Context
PR Compliance ID 1 requires that behavior changes remain verifiable and discoverable via updates to domain manual test docs. The manual test docs should include prerequisites, exact actions/steps, expected results, and rollback/cleanup notes when applicable—especially for user-visible rendering changes and navigation/unmount scenarios.
## Fix Focus Areas
- tests/chat-composer-rendering/index.md[7-43]
- tests/skills-plugins-integrations/index.md[7-37]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+51
to
+66
| // Remove dangerous attributes (event handlers, javascript: URLs) | ||
| const attrs = el.attributes | ||
| for (let i = attrs.length - 1; i >= 0; i--) { | ||
| const attrName = attrs[i].name.toLowerCase() | ||
| const attrValue = attrs[i].value.toLowerCase() | ||
|
|
||
| if (attrName.startsWith('on')) { | ||
| el.removeAttribute(attrs[i].name) | ||
| continue | ||
| } | ||
|
|
||
| if (attrName === 'href' || attrName === 'src' || attrName === 'action') { | ||
| if (attrValue.startsWith('javascript:') || attrValue.startsWith('vbscript:') || attrValue.startsWith('data:')) { | ||
| el.removeAttribute(attrs[i].name) | ||
| } | ||
| } |
There was a problem hiding this comment.
2. Url scheme bypass 🐞 Bug ⛨ Security
sanitizeHtml() checks href/src/action with startsWith() on the raw attribute value without trimming/normalizing, so values like " \njavascript:..." bypass the filter and remain in the output. Because the sanitized result is injected via v-html (e.g., SkillDetailModal/ThreadConversation), this can re-enable XSS via crafted links/images.
Agent Prompt
## Issue description
`sanitizeHtml()` blocks `javascript:`, `vbscript:`, and `data:` URLs only when the attribute value *starts with* those strings. Because the current implementation does not trim/strip leading whitespace/control characters (and doesn’t otherwise normalize URLs), dangerous schemes can bypass the check.
## Issue Context
The sanitizer output is used in `v-html` sinks (e.g., `SkillDetailModal.vue` and `ThreadConversation.vue`), so bypasses directly affect DOM insertion.
## Fix Focus Areas
- src/utils/sanitizeHtml.ts[51-66]
## Suggested fix
- Normalize attribute values before checking schemes (at minimum: `const normalized = attrs[i].value.trim().toLowerCase()`), and consider stripping ASCII control chars (`\u0000`-`\u001F`, `\u007F`) before evaluation.
- Prefer a protocol-based check using `new URL(value, base)` where applicable and reject any non-allowlisted protocols (e.g., allow `http:`, `https:`, `mailto:`, `tel:` for `href`; allow `http:`/`https:` for `src`).
- If you keep string checks, ensure they handle leading whitespace and mixed whitespace like `"\t\njavascript:"`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/content/ThreadConversation.vue (1)
3629-3633: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSanitize once at the final render boundary.
renderMarkdownBlocksAsHtml()already sanitizes the fullrawHtmlbefore caching, so sanitizing each code/list fragment adds repeated blockingDOMParserwork for large messages.Proposed refactor
function renderCachedHighlightedCodeAsHtml(language: string, value: string): string { - return sanitizeHtml(_renderCachedHighlightedCodeAsHtml(language, value)) -} - -function _renderCachedHighlightedCodeAsHtml(language: string, value: string): string { const cacheKey = `${highlightCacheVersion.value}\u0000${normalizeCodeLanguage(language)}\u0000${language}\u0000${value}` const cached = highlightHtmlCache.get(cacheKey) if (cached !== undefined) { @@ function renderListItemContentAsHtml(item: ListItem): string { const paragraphsHtml = renderListItemParagraphsAsHtml(item) const childrenHtml = item.children?.map((block) => renderMessageBlockAsHtml(block)).join('') ?? '' - return sanitizeHtml(paragraphsHtml + childrenHtml) + return paragraphsHtml + childrenHtml }Also applies to: 3681-3684, 3762-3765
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/content/ThreadConversation.vue` around lines 3629 - 3633, The highlighted code/list fragment helpers are sanitizing too early and duplicating expensive DOMParser work because renderMarkdownBlocksAsHtml already sanitizes the full HTML before caching. Update renderCachedHighlightedCodeAsHtml, renderCachedMarkdownListAsHtml, and the related fragment render helper paths so they return raw rendered HTML and rely on the final render boundary to sanitize once, keeping the existing caching behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/content/DirectoryHub.vue`:
- Around line 879-882: The timer cleanup in DirectoryHub.vue is incomplete
because async work can still call showToast() or mutate state after unmount. Add
a component-level disposed flag such as isUnmounted, set it in
onBeforeUnmount(), and have showToast plus any post-await state updates in this
component check that flag before creating new timeouts or writing state.
Reference the existing showToast function and the onBeforeUnmount teardown so
the guard covers all late-firing async paths.
In `@src/components/content/SkillsHub.vue`:
- Around line 176-178: The toast cleanup in SkillsHub is incomplete because
onBeforeUnmount only clears the current timer, but async handlers can still call
showToast() after teardown and recreate it. Add an isUnmounted guard in
showToast and any post-await flows in the same component so no new timeout or
state update is scheduled once unmounted. Use the existing showToast and
onBeforeUnmount logic in SkillsHub.vue as the places to wire this guard and
prevent late resolves from re-arming the timer.
In `@src/utils/sanitizeHtml.ts`:
- Around line 23-24: The fallback in sanitizeHtml currently returns the original
HTML when DOMParser is unavailable, so make it fail closed by escaping or
neutralizing the input instead of passing it through unchanged. In sanitizeHtml
and the URL attribute checks around the scheme validation logic, normalize
candidate URLs by stripping whitespace and control characters before testing for
dangerous schemes, so payloads like href with embedded newlines or tabs are
caught consistently. Use the existing sanitizeHtml flow and its attribute
filtering helpers to apply the fix in one place, including the href/src scheme
handling path.
---
Nitpick comments:
In `@src/components/content/ThreadConversation.vue`:
- Around line 3629-3633: The highlighted code/list fragment helpers are
sanitizing too early and duplicating expensive DOMParser work because
renderMarkdownBlocksAsHtml already sanitizes the full HTML before caching.
Update renderCachedHighlightedCodeAsHtml, renderCachedMarkdownListAsHtml, and
the related fragment render helper paths so they return raw rendered HTML and
rely on the final render boundary to sanitize once, keeping the existing caching
behavior intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d6676400-339e-4716-8d29-f0d48a2b42e2
📒 Files selected for processing (6)
src/components/SafeHtml.vuesrc/components/content/DirectoryHub.vuesrc/components/content/SkillDetailModal.vuesrc/components/content/SkillsHub.vuesrc/components/content/ThreadConversation.vuesrc/utils/sanitizeHtml.ts
Comment on lines
+879
to
+882
| onBeforeUnmount(() => { | ||
| if (toastTimer) clearTimeout(toastTimer) | ||
| if (composioSearchTimer) clearTimeout(composioSearchTimer) | ||
| }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cleanup is incomplete without a disposed guard.
This clears timers that already exist, but async actions in this component can still finish after unmount and call showToast(), which creates a brand-new timeout on a destroyed instance. Guard showToast/post-await state writes with an isUnmounted flag so teardown cannot recreate the leak you’re trying to fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/content/DirectoryHub.vue` around lines 879 - 882, The timer
cleanup in DirectoryHub.vue is incomplete because async work can still call
showToast() or mutate state after unmount. Add a component-level disposed flag
such as isUnmounted, set it in onBeforeUnmount(), and have showToast plus any
post-await state updates in this component check that flag before creating new
timeouts or writing state. Reference the existing showToast function and the
onBeforeUnmount teardown so the guard covers all late-firing async paths.
Comment on lines
+176
to
+178
| onBeforeUnmount(() => { | ||
| if (toastTimer) clearTimeout(toastTimer) | ||
| }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The timer can still be recreated after unmount.
onBeforeUnmount only cancels the current handle. If an in-flight install/search/toggle request resolves later, its showToast() call will schedule a new timeout after teardown. Please pair this with an isUnmounted guard in showToast (and any other post-await state updates) to actually close the leak.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/content/SkillsHub.vue` around lines 176 - 178, The toast
cleanup in SkillsHub is incomplete because onBeforeUnmount only clears the
current timer, but async handlers can still call showToast() after teardown and
recreate it. Add an isUnmounted guard in showToast and any post-await flows in
the same component so no new timeout or state update is scheduled once
unmounted. Use the existing showToast and onBeforeUnmount logic in SkillsHub.vue
as the places to wire this guard and prevent late resolves from re-arming the
timer.
Comment on lines
+23
to
+24
| export function sanitizeHtml(raw: string): string { | ||
| if (typeof DOMParser === 'undefined') return raw |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make the sanitizer fail closed and normalize URL schemes.
Line 24 returns unsanitized HTML when DOMParser is unavailable, and Lines 55-64 miss payloads such as href="\njavascript:...". Escape on fallback and strip whitespace/control characters before checking dangerous schemes.
Proposed fix
const DENIED_TAGS = new Set([
'script', 'style', 'iframe', 'frame', 'object', 'embed', 'param',
'applet', 'link', 'base', 'meta', 'noscript',
])
+const URL_ATTRIBUTES = new Set(['href', 'src', 'action', 'formaction', 'xlink:href'])
+
+function escapeHtml(raw: string): string {
+ return raw
+ .replace(/&/gu, '&')
+ .replace(/</gu, '<')
+ .replace(/>/gu, '>')
+ .replace(/"/gu, '"')
+ .replace(/'/gu, '&`#39`;')
+}
+
+function isUnsafeUrl(value: string): boolean {
+ const normalized = value.replace(/[\u0000-\u001F\u007F\s]+/gu, '').toLowerCase()
+ return /^(?:javascript|vbscript|data):/u.test(normalized)
+}
+
/**
* Sanitize an HTML string in the browser.
* Parses the HTML, strips forbidden tags and attributes, and returns clean HTML.
*/
export function sanitizeHtml(raw: string): string {
- if (typeof DOMParser === 'undefined') return raw
+ if (typeof DOMParser === 'undefined') return escapeHtml(raw)
if (!raw) return ''
@@
- const attrValue = attrs[i].value.toLowerCase()
-
if (attrName.startsWith('on')) {
el.removeAttribute(attrs[i].name)
continue
}
- if (attrName === 'href' || attrName === 'src' || attrName === 'action') {
- if (attrValue.startsWith('javascript:') || attrValue.startsWith('vbscript:') || attrValue.startsWith('data:')) {
+ if (URL_ATTRIBUTES.has(attrName)) {
+ if (isUnsafeUrl(attrs[i].value)) {
el.removeAttribute(attrs[i].name)
}
}Also applies to: 51-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/sanitizeHtml.ts` around lines 23 - 24, The fallback in sanitizeHtml
currently returns the original HTML when DOMParser is unavailable, so make it
fail closed by escaping or neutralizing the input instead of passing it through
unchanged. In sanitizeHtml and the URL attribute checks around the scheme
validation logic, normalize candidate URLs by stripping whitespace and control
characters before testing for dangerous schemes, so payloads like href with
embedded newlines or tabs are caught consistently. Use the existing sanitizeHtml
flow and its attribute filtering helpers to apply the fix in one place,
including the href/src scheme handling path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🐛 Bugs Fixed
Medium: Timer Leaks
onBeforeUnmountto cleartoastTimerandcomposioSearchTimerwhen the component unmounts before timers fireonBeforeUnmountto cleartoastTimeron unmountLow Risk: HTML Sanitization (Defense in Depth)
src/utils/sanitizeHtml.ts: Lightweight native DOMParser-based HTML sanitizer that strips<script>,<iframe>,<object>, event-handler attributes (onclick,onerror, etc.), andjavascript:URLs. No external dependencies.src/components/SafeHtml.vue: Reusable Vue component wrappingsanitizeHtmlsanitizeHtml()torenderMarkdownBlocksAsHtml(),renderListItemContentAsHtml(), andrenderCachedHighlightedCodeAsHtml()— all three v-html entry pointssanitizeHtml()torenderedReadmecomputed✅ Verification
v-forloops verified to have:key(42+ instances across codebase — all present)DOMParserAPI)Summary by CodeRabbit
New Features
Bug Fixes