Fix stored XSS in asset/sub-asset rendering (CVE-2026-45231) - #135
Open
YoyoChaud wants to merge 1 commit into
Open
Fix stored XSS in asset/sub-asset rendering (CVE-2026-45231)#135YoyoChaud wants to merge 1 commit into
YoyoChaud wants to merge 1 commit into
Conversation
Escape user-controlled fields at the render layer so existing entries are protected and edit/save round-trips don't accumulate HTML entities. Adds src/services/render/escape.js with escapeHtml() and safeUrl(), imported by every site that interpolates asset/sub-asset data into innerHTML: assetRenderer (details, info, file grid, maintenance events), listRenderer (sidebar list), previewRenderer (file previews), script.js (sub-asset list and nested children, tag manager) and dashboardManager (events table). safeUrl() neutralizes javascript:/data:/vbscript: URIs in asset.link before it lands in href.
WalkthroughThis PR applies HTML escaping and URL sanitization across six rendering modules to prevent XSS attacks. A new ChangesHTML Escaping and XSS Prevention
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR hardens client-side rendering against XSS by introducing centralized HTML escaping helpers and applying them across preview/list/detail/dashboard rendering where innerHTML is used.
Changes:
- Added
escapeHtml()andsafeUrl()helpers for safely interpolating values into HTML strings. - Updated multiple renderers to escape user-controlled fields (names, tags, notes, IDs) before inserting into
innerHTML. - Applied URL sanitization for
asset.linkin the asset details view.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/render/previewRenderer.js | Escapes preview src and file name when rendering previews via innerHTML. |
| src/services/render/listRenderer.js | Escapes asset list name/model/tags before injecting into sidebar HTML. |
| src/services/render/escape.js | Adds HTML escaping and basic URL scheme filtering helpers. |
| src/services/render/assetRenderer.js | Escapes many asset fields (including tags/notes) and sanitizes asset.link href. |
| public/script.js | Escapes sub-asset names/IDs/tags and file URLs in dynamically generated HTML. |
| public/managers/dashboardManager.js | Escapes event fields and IDs in dashboard event list HTML. |
| if (value === null || value === undefined) return '#'; | ||
| const s = String(value).trim(); | ||
| if (!s) return '#'; | ||
| if (/^(javascript|data|vbscript):/i.test(s)) return '#'; |
Comment on lines
+269
to
+271
| <a href="${escapeHtml(formatFilePath(photoPath))}" target="_blank" class="file-preview"> | ||
| <img src="${escapeHtml(formatFilePath(photoPath))}" alt="${escapeHtml(asset.name)}" class="asset-image"> | ||
| <div class="file-label">${escapeHtml(formatDisplayFileName(fileName))}</div> |
| </button> | ||
| <div class="file-info-pill"> | ||
| <span class="file-name">${fileName}</span> | ||
| <span class="file-name">${escapeHtml(fileName)}</span> |
| <img src="${formatFilePath(asset.photoPath)}" alt="${asset.name}" class="asset-image"> | ||
| <div class="file-label">${formatDisplayFileName(fileName)}</div> | ||
| <a href="${escapeHtml(formatFilePath(asset.photoPath))}" target="_blank" class="file-preview"> | ||
| <img src="${escapeHtml(formatFilePath(asset.photoPath))}" alt="${escapeHtml(asset.name)}" class="asset-image"> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/services/render/listRenderer.js (1)
339-339: ⚡ Quick winWrap mapped tag markup to keep the line-length rule.
The current one-liner exceeds the 100-character limit and hurts readability.
♻️ Proposed refactor
- ${asset.tags.map(tag => `<span class="asset-tag" data-tag="${escapeHtml(tag)}">${escapeHtml(tag)}</span>`).join('')} + ${asset.tags + .map(tag => ` + <span class="asset-tag" data-tag="${escapeHtml(tag)}"> + ${escapeHtml(tag)} + </span> + `) + .join('')}As per coding guidelines, "
{src,public}/**/*.js: Maximum line length is 100 characters in JavaScript files".🤖 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/services/render/listRenderer.js` at line 339, The template expression generating tag spans is a single long line: the asset.tags.map(...) call that uses escapeHtml to render each `<span class="asset-tag" data-tag="...">...</span>` exceeds the 100-char rule; refactor by breaking the mapping into multiple lines or extracting it to a named variable (e.g., tagHtml = asset.tags.map(tag => { ... }).join('')) so the arrow callback and returned template string are multi-line and indented, keeping references to asset.tags, escapeHtml, the "asset-tag" class and data-tag attribute intact.src/services/render/assetRenderer.js (1)
216-216: ⚡ Quick winSplit oversized template literals to stay within line-length limits.
These changed lines are hard to scan and exceed the 100-character max. Please wrap them into multiline template fragments.
♻️ Proposed refactor
- <div><a href="${escapeHtml(safeUrl(asset.link))}" target="_blank" rel="noopener noreferrer">${escapeHtml(asset.link)}</a></div> + <div> + <a + href="${escapeHtml(safeUrl(asset.link))}" + target="_blank" + rel="noopener noreferrer" + > + ${escapeHtml(asset.link)} + </a> + </div>- ${asset.tags.map(tag => `<span class="tag" data-tag="${escapeHtml(tag)}" style="cursor: pointer;">${escapeHtml(tag)}</span>`).join('')} + ${asset.tags + .map(tag => ` + <span class="tag" data-tag="${escapeHtml(tag)}" style="cursor: pointer;"> + ${escapeHtml(tag)} + </span> + `) + .join('')}As per coding guidelines, "
{src,public}/**/*.js: Maximum line length is 100 characters in JavaScript files".Also applies to: 494-494
🤖 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/services/render/assetRenderer.js` at line 216, The long template literal for the anchor tag in the render output (uses escapeHtml(safeUrl(asset.link)) and escapeHtml(asset.link)) exceeds the 100-char limit; refactor by extracting values (e.g., const safe = escapeHtml(safeUrl(asset.link)); const label = escapeHtml(asset.link)) and then rebuild the template using multiline template fragments or concatenation so the <a href="${...}" target="_blank" rel="noopener noreferrer"> and its inner text appear on separate lines; update the code paths that render this anchor (the block in assetRenderer.js that constructs the div/a) to use these extracted symbols and keep each line under 100 chars.
🤖 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 `@public/script.js`:
- Line 16: Replace usage of escapeHtml(...) for URL sinks with safeUrl(...) and
import safeUrl from the escape module: update the import line to include safeUrl
(e.g., import { escapeHtml, safeUrl } from '/src/services/render/escape.js') and
then change escapeHtml(formatFilePath(...)) to safeUrl(formatFilePath(...)) for
the URL attributes referencing subAsset.photoPath (href and src),
subAsset.receiptPath (href), subAsset.manualPath (href), child.photoPath (href
and src), child.receiptPath (href), and child.manualPath (href); keep escapeHtml
for non-URL HTML text sinks.
In `@src/services/render/escape.js`:
- Around line 9-25: Add JSDoc blocks for the two public exports to document
their API: add a JSDoc comment above escapeHtml describing the function, its
parameter (value: any, nullable/optional), and that it returns a string with
HTML characters escaped (use `@param` and `@returns`), and add a JSDoc comment above
safeUrl describing the parameter (value: any, nullable/optional), its behavior
(trims input, returns '#' for empty or disallowed schemes like javascript:,
data:, vbscript:), and that it returns a safe URL string (use `@param` and
`@returns`); ensure the comments use the project’s JSDoc style for exported/public
helpers so they appear in generated docs.
---
Nitpick comments:
In `@src/services/render/assetRenderer.js`:
- Line 216: The long template literal for the anchor tag in the render output
(uses escapeHtml(safeUrl(asset.link)) and escapeHtml(asset.link)) exceeds the
100-char limit; refactor by extracting values (e.g., const safe =
escapeHtml(safeUrl(asset.link)); const label = escapeHtml(asset.link)) and then
rebuild the template using multiline template fragments or concatenation so the
<a href="${...}" target="_blank" rel="noopener noreferrer"> and its inner text
appear on separate lines; update the code paths that render this anchor (the
block in assetRenderer.js that constructs the div/a) to use these extracted
symbols and keep each line under 100 chars.
In `@src/services/render/listRenderer.js`:
- Line 339: The template expression generating tag spans is a single long line:
the asset.tags.map(...) call that uses escapeHtml to render each `<span
class="asset-tag" data-tag="...">...</span>` exceeds the 100-char rule; refactor
by breaking the mapping into multiple lines or extracting it to a named variable
(e.g., tagHtml = asset.tags.map(tag => { ... }).join('')) so the arrow callback
and returned template string are multi-line and indented, keeping references to
asset.tags, escapeHtml, the "asset-tag" class and data-tag attribute 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49fdb68f-51fd-42a6-82c4-e62b1f1ef79b
📒 Files selected for processing (6)
public/managers/dashboardManager.jspublic/script.jssrc/services/render/assetRenderer.jssrc/services/render/escape.jssrc/services/render/listRenderer.jssrc/services/render/previewRenderer.js
| // Import file upload module | ||
| import { initializeFileUploads, handleFileUploads } from '/src/services/fileUpload/index.js'; | ||
| import { formatFileSize } from '/src/services/fileUpload/utils.js'; | ||
| import { escapeHtml } from '/src/services/render/escape.js'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== formatFilePath definition(s) =="
rg -n --type=js -C3 '\bformatFilePath\b'
echo
echo "== URL sinks using formatFilePath =="
rg -n --type=js -C2 'href="\$\{[^}]*formatFilePath\(|src="\$\{[^}]*formatFilePath\('
echo
echo "== Existing safeUrl usage with formatFilePath =="
rg -n --type=js -C2 'safeUrl\(\s*formatFilePath\('Repository: DumbWareio/DumbAssets
Length of output: 29096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get full formatFilePath implementation
echo "== Full formatFilePath implementation =="
sed -n '97,120p' src/services/render/assetRenderer.js
echo
echo "== escape.js exports and safeUrl check =="
cat src/services/render/escape.js | head -100Repository: DumbWareio/DumbAssets
Length of output: 1750
Use safeUrl() for URL sinks (href/src), not escapeHtml().
Your code pipes escapeHtml(formatFilePath(...)) into href and src attributes. Problem: escapeHtml() only escapes HTML characters; it doesn't block dangerous URL schemes. If formatFilePath() returns a URL starting with http:// or https://, it passes through unmodified. A malicious URL like https://example.com/javascript:alert(1) bypasses escapeHtml entirely and reaches the browser's URL parser.
The safeUrl() function already exists in your escape module and explicitly neutralizes javascript:, data:, and vbscript: schemes. Use it for all URL attributes.
Fix
-import { escapeHtml } from '/src/services/render/escape.js';
+import { escapeHtml, safeUrl } from '/src/services/render/escape.js';Then replace escapeHtml(formatFilePath(...)) with safeUrl(formatFilePath(...)) in these URL sinks:
- Lines 881-882:
hrefandsrcfor subAsset.photoPath - Line 891:
hreffor subAsset.receiptPath - Line 907:
hreffor subAsset.manualPath - Lines 1032-1033:
hrefandsrcfor child.photoPath - Line 1042:
hreffor child.receiptPath - Line 1058:
hreffor child.manualPath
🤖 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 `@public/script.js` at line 16, Replace usage of escapeHtml(...) for URL sinks
with safeUrl(...) and import safeUrl from the escape module: update the import
line to include safeUrl (e.g., import { escapeHtml, safeUrl } from
'/src/services/render/escape.js') and then change
escapeHtml(formatFilePath(...)) to safeUrl(formatFilePath(...)) for the URL
attributes referencing subAsset.photoPath (href and src), subAsset.receiptPath
(href), subAsset.manualPath (href), child.photoPath (href and src),
child.receiptPath (href), and child.manualPath (href); keep escapeHtml for
non-URL HTML text sinks.
Comment on lines
+9
to
+25
| export function escapeHtml(value) { | ||
| if (value === null || value === undefined) return ''; | ||
| return String(value) | ||
| .replace(/&/g, '&') | ||
| .replace(/</g, '<') | ||
| .replace(/>/g, '>') | ||
| .replace(/"/g, '"') | ||
| .replace(/'/g, '''); | ||
| } | ||
|
|
||
| export function safeUrl(value) { | ||
| if (value === null || value === undefined) return '#'; | ||
| const s = String(value).trim(); | ||
| if (!s) return '#'; | ||
| if (/^(javascript|data|vbscript):/i.test(s)) return '#'; | ||
| return s; | ||
| } |
There was a problem hiding this comment.
Add JSDoc for exported helpers.
escapeHtml and safeUrl are public exports and should include JSDoc blocks for API clarity and consistency.
♻️ Proposed fix
+/**
+ * Escape HTML-special characters for safe interpolation into HTML strings.
+ * `@param` {*} value Value to escape.
+ * `@returns` {string} Escaped HTML string.
+ */
export function escapeHtml(value) {
if (value === null || value === undefined) return '';
return String(value)
.replace(/&/g, '&')
.replace(/</g, '<')
.replace(/>/g, '>')
.replace(/"/g, '"')
.replace(/'/g, '&`#039`;');
}
+/**
+ * Normalize and sanitize URL values used in href/src attributes.
+ * `@param` {*} value URL candidate value.
+ * `@returns` {string} Safe URL string or "#" when blocked/empty.
+ */
export function safeUrl(value) {
if (value === null || value === undefined) return '#';
const s = String(value).trim();
if (!s) return '#';
if (/^(javascript|data|vbscript):/i.test(s)) return '#';
return s;
}As per coding guidelines, "**/*.{js,mjs}: JSDoc comments for public functions and APIs".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function escapeHtml(value) { | |
| if (value === null || value === undefined) return ''; | |
| return String(value) | |
| .replace(/&/g, '&') | |
| .replace(/</g, '<') | |
| .replace(/>/g, '>') | |
| .replace(/"/g, '"') | |
| .replace(/'/g, '''); | |
| } | |
| export function safeUrl(value) { | |
| if (value === null || value === undefined) return '#'; | |
| const s = String(value).trim(); | |
| if (!s) return '#'; | |
| if (/^(javascript|data|vbscript):/i.test(s)) return '#'; | |
| return s; | |
| } | |
| /** | |
| * Escape HTML-special characters for safe interpolation into HTML strings. | |
| * `@param` {*} value Value to escape. | |
| * `@returns` {string} Escaped HTML string. | |
| */ | |
| export function escapeHtml(value) { | |
| if (value === null || value === undefined) return ''; | |
| return String(value) | |
| .replace(/&/g, '&') | |
| .replace(/</g, '<') | |
| .replace(/>/g, '>') | |
| .replace(/"/g, '"') | |
| .replace(/'/g, '&`#039`;'); | |
| } | |
| /** | |
| * Normalize and sanitize URL values used in href/src attributes. | |
| * `@param` {*} value URL candidate value. | |
| * `@returns` {string} Safe URL string or "#" when blocked/empty. | |
| */ | |
| export function safeUrl(value) { | |
| if (value === null || value === undefined) return '#'; | |
| const s = String(value).trim(); | |
| if (!s) return '#'; | |
| if (/^(javascript|data|vbscript):/i.test(s)) return '#'; | |
| return s; | |
| } |
🤖 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/services/render/escape.js` around lines 9 - 25, Add JSDoc blocks for the
two public exports to document their API: add a JSDoc comment above escapeHtml
describing the function, its parameter (value: any, nullable/optional), and that
it returns a string with HTML characters escaped (use `@param` and `@returns`), and
add a JSDoc comment above safeUrl describing the parameter (value: any,
nullable/optional), its behavior (trims input, returns '#' for empty or
disallowed schemes like javascript:, data:, vbscript:), and that it returns a
safe URL string (use `@param` and `@returns`); ensure the comments use the project’s
JSDoc style for exported/public helpers so they appear in generated docs.
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
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.
Escape user-controlled fields at the render layer so existing entries are protected and edit/save round-trips don't accumulate HTML entities.
Adds src/services/render/escape.js with escapeHtml() and safeUrl(), imported by every site that interpolates asset/sub-asset data into innerHTML: assetRenderer (details, info, file grid, maintenance events), listRenderer (sidebar list), previewRenderer (file previews), script.js (sub-asset list and nested children, tag manager) and dashboardManager (events table). safeUrl() neutralizes javascript:/data:/vbscript: URIs in asset.link before it lands in href.
High-level PR Summary
This PR fixes a stored XSS vulnerability (CVE-2026-45231) in asset and sub-asset rendering by introducing HTML escaping and URL sanitization functions. A new utility module
escape.jsprovidesescapeHtml()to encode special characters andsafeUrl()to neutralize dangerous URI schemes likejavascript:,data:, andvbscript:. These functions are applied throughout all rendering layers where user-controlled asset data (names, descriptions, tags, model numbers, serial numbers, event details, file paths, and links) is interpolated intoinnerHTML, protecting against XSS attacks while preserving existing database entries without requiring double-encoding.⏱️ Estimated Review Time: 30-90 minutes
💡 Review Order Suggestion
src/services/render/escape.jssrc/services/render/assetRenderer.jssrc/services/render/listRenderer.jssrc/services/render/previewRenderer.jspublic/script.jspublic/managers/dashboardManager.jsSummary by CodeRabbit
Release Notes