Security: Remote-code-capable XSS via unsanitized playlist metadata rendered with innerHTML#420
Conversation
…ed playlist m Playlist entry fields (e.g., `entry.title`, thumbnail URL, entry URL) from yt-dlp JSON are concatenated into HTML and assigned with `innerHTML`. Metadata from remote sites can include HTML/JS payloads. In this app, renderers have Node integration enabled and context isolation disabled, so DOM XSS can become full RCE. Affected files: playlist_new.js Signed-off-by: tuanaiseo <221258316+tuanaiseo@users.noreply.github.com>
WalkthroughThe pull request adds URL sanitization and refactors playlist item rendering in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/playlist_new.js (2)
86-148:⚠️ Potential issue | 🟡 MinorDOM construction approach is correct and closes the XSS sink.
All untrusted fields (
entry.title,entry.url,entry.thumbnails[3].url) are now assigned viatextContent,.value(property, not attribute), or a sanitizedsrc, so no value reaches an HTML parser. Clearingdataviadata.textContent = ""before appending the fragment and batching appends into aDocumentFragmentare both good choices.Two minor, non-blocking observations in this block:
- Line 92:
console.log(entry)is a debug artifact and logs full remote metadata per render — consider removing before merge.- Line 115:
image.alt = "No thumbnail";is hardcoded, whereas surrounding user-facing strings go throughi18n.__(). Considerimage.alt = i18n.__("No thumbnail");for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/playlist_new.js` around lines 86 - 148, Remove the debug console output and internationalize the hardcoded alt text: delete the console.log(entry) call inside the parsed.entries.forEach loop and replace the image.alt = "No thumbnail"; assignment with image.alt = i18n.__("No thumbnail"); (ensure i18n is in scope). These changes touch the forEach handler that processes entry objects and the image element creation code where console.log(entry) and image.alt are set.
1-229:⚠️ Potential issue | 🟠 MajorPR description vs. diff mismatch: Electron hardening not implemented.
The PR description states the solution hardens Electron by setting
nodeIntegration: falseandcontextIsolation: true. However,main.jsstill has both set to their unsafe defaults (nodeIntegration: true,contextIsolation: false), and no changes to the BrowserWindowwebPreferencesappear in this diff.Given the stated threat model (DOM XSS escalates to RCE due to Node integration being enabled and context isolation being disabled), the renderer-side sanitization in
src/playlist_new.jsalone is only defense-in-depth. The actual RCE mitigation requires theBrowserWindowhardening. Please either:
- Include the required
webPreferenceschanges inmain.js, or- Update the PR description to accurately reflect what this change addresses (preventing DOM injection in the renderer, not eliminating the RCE risk).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/playlist_new.js` around lines 1 - 229, The PR claims Electron hardening (nodeIntegration: false, contextIsolation: true) but the diff only changes renderer code (functions like sanitizeHttpUrl and pasteLink in playlist_new.js) and does not update the BrowserWindow creation in main.js; either modify main.js to set webPreferences on the BrowserWindow (set nodeIntegration: false and contextIsolation: true) so the stated RCE mitigation is actually implemented, or update the PR description to state this change is defense-in-depth for renderer DOM sanitization only (mentioning sanitizeHttpUrl/pasteLink) and does not remove the elevated-risk configuration in BrowserWindow (BrowserWindow, webPreferences, nodeIntegration, contextIsolation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/playlist_new.js`:
- Line 80: Multiple sibling locations still assign untrusted URL values into
innerHTML (e.g., errorDetails.innerHTML, this.ui.errorDetails.innerHTML) causing
DOM XSS; change those assignments to use textContent (or properly
escape/sanitize the interpolated values) instead of innerHTML so the URL/url
variables are not interpreted as HTML. Locate the occurrences that use
errorDetails.innerHTML and this.ui.errorDetails.innerHTML and replace the HTML
interpolation with plain text assignments (or call a safe-escape helper) to
ensure the untrusted this.state.url / url values are rendered as text.
---
Outside diff comments:
In `@src/playlist_new.js`:
- Around line 86-148: Remove the debug console output and internationalize the
hardcoded alt text: delete the console.log(entry) call inside the
parsed.entries.forEach loop and replace the image.alt = "No thumbnail";
assignment with image.alt = i18n.__("No thumbnail"); (ensure i18n is in scope).
These changes touch the forEach handler that processes entry objects and the
image element creation code where console.log(entry) and image.alt are set.
- Around line 1-229: The PR claims Electron hardening (nodeIntegration: false,
contextIsolation: true) but the diff only changes renderer code (functions like
sanitizeHttpUrl and pasteLink in playlist_new.js) and does not update the
BrowserWindow creation in main.js; either modify main.js to set webPreferences
on the BrowserWindow (set nodeIntegration: false and contextIsolation: true) so
the stated RCE mitigation is actually implemented, or update the PR description
to state this change is defense-in-depth for renderer DOM sanitization only
(mentioning sanitizeHttpUrl/pasteLink) and does not remove the elevated-risk
configuration in BrowserWindow (BrowserWindow, webPreferences, nodeIntegration,
contextIsolation).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| <br><br> | ||
| ${error} | ||
| `; | ||
| getId("errorDetails").textContent = `URL: ${clipboardText}\n\n${error}`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the innerHTML-with-URL sink still exists in the sibling files and find any other instances.
rg -nP --type=js -C2 'innerHTML\s*=\s*`[^`]*\$\{[^}]*\b(url|URL)\b'
rg -nP --type=js -C2 '\.innerHTML\s*=' -g '!**/node_modules/**'Repository: aandrew-me/ytDownloader
Length of output: 3730
Sibling files still contain the same DOM XSS vulnerability.
This PR fixes the vulnerability in src/playlist_new.js by switching from innerHTML to textContent, but the identical vulnerable pattern remains in:
src/renderer.js:1557:errorDetails.innerHTML = \URL: ${url}...``src/playlist.js:584–586:this.ui.errorDetails.innerHTML = \URL: ${this.state.url}...``
Both files directly interpolate untrusted URL/url variables into HTML. If the PR treats this as critical (DOM XSS → RCE via Node integration), the fix should extend to these sibling locations or be tracked as a follow-up issue linked to this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/playlist_new.js` at line 80, Multiple sibling locations still assign
untrusted URL values into innerHTML (e.g., errorDetails.innerHTML,
this.ui.errorDetails.innerHTML) causing DOM XSS; change those assignments to use
textContent (or properly escape/sanitize the interpolated values) instead of
innerHTML so the URL/url variables are not interpreted as HTML. Locate the
occurrences that use errorDetails.innerHTML and this.ui.errorDetails.innerHTML
and replace the HTML interpolation with plain text assignments (or call a
safe-escape helper) to ensure the untrusted this.state.url / url values are
rendered as text.
|
playlist_new.js file isn't even used in the app |
2 participants
Problem
Playlist entry fields (e.g.,
entry.title, thumbnail URL, entry URL) from yt-dlp JSON are concatenated into HTML and assigned withinnerHTML. Metadata from remote sites can include HTML/JS payloads. In this app, renderers have Node integration enabled and context isolation disabled, so DOM XSS can become full RCE.Severity:
criticalFile:
src/playlist_new.jsSolution
Never inject untrusted content with
innerHTML. Build DOM nodes withcreateElement, assigntextContent/setAttributeafter strict validation, and sanitize URLs. Additionally harden Electron (disable Node integration, enable context isolation).Changes
src/playlist_new.js(modified)Testing
Summary by CodeRabbit