-
Notifications
You must be signed in to change notification settings - Fork 2
feat: normalize numeric formatting & add time-range selector for usage trends (sanitization fix) #60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| html += '</div>'; | ||
|
|
||
| container.innerHTML = html; |
Check failure
Code scanning / CodeQL
Client-side cross-site scripting High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the DOM-based XSS vulnerability, all instances where untrusted data is inserted into HTML must be properly escaped before being included in HTML strings set via innerHTML. In this case, the affected code is in the renderMultiMonthAnalysis function, especially where values from the analysis object (ultimately from event.data) are interpolated into the html string.
The safest and least intrusive fix is to create a reusable HTML-escaping function, such as escapeHtml, and apply it to any untrusted values inserted into the HTML string. This method leaves all layout and logic unchanged, only ensuring that inserted content cannot break HTML context or inject scripts. The escapeHtml function can be added near the top of the module, or immediately before its first use.
Approach
- Add a helper function (e.g.,
escapeHtml(str)) that escapes<,>,&,",', and`. - In
renderMultiMonthAnalysis, wrap all instances ofanalysis.dataMonths(and any other untrusted values) insideescapeHtml(String(...))when interpolating into the HTML string. - (If there are other places where untrusted data is injected into HTML in this block, escape those as well; focus on what is shown in the snippet.)
- No changes needed to dependencies—basic escaping can be accomplished with a simple utility.
-
Copy modified lines R1103-R1113 -
Copy modified line R1141
| @@ -1100,6 +1100,17 @@ | ||
| } | ||
| }); | ||
|
|
||
| // Escapes user-provided values before injecting into HTML to prevent XSS | ||
| function escapeHtml(str) { | ||
| return String(str) | ||
| .replace(/&/g, '&') | ||
| .replace(/</g, '<') | ||
| .replace(/>/g, '>') | ||
| .replace(/"/g, '"') | ||
| .replace(/'/g, ''') | ||
| .replace(/`/g, '`'); | ||
| } | ||
|
|
||
| function renderMultiMonthAnalysis(analysis) { | ||
| try { log('[renderMultiMonthAnalysis] called with: ' + JSON.stringify(analysis)); } catch { } | ||
|
|
||
| @@ -1127,7 +1138,7 @@ | ||
| // Build the HTML | ||
| let html = '<h3>Multi-Month Analysis</h3>'; | ||
| html += '<div class="analysis-summary">'; | ||
| html += '<p><strong>Analysis Period:</strong> ' + analysis.dataMonths + ' month' + (analysis.dataMonths > 1 ? 's' : '') + ' of data</p>'; | ||
| html += '<p><strong>Analysis Period:</strong> ' + escapeHtml(analysis.dataMonths) + ' month' + (Number(analysis.dataMonths) > 1 ? 's' : '') + ' of data</p>'; | ||
|
|
||
| // Growth Trends | ||
| if (analysis.growthTrends && analysis.growthTrends.length > 0) { |
…d param with underscore
…ode-scanning flags
Address security/code-scanning/9: Sanitize dynamic HTML content in the webview and avoid unsanitized
innerHTMLto reduce DOM XSS risk. Also fixed linter issues insrc/lib/usageHistory.ts(removed unused async/unused params).\n\nChanges include:\n- AddedescapeHtml()helper inmedia/webview.jsand used it when interpolating user-controlled or extension-provided strings intoinnerHTMLtemplates (insights, anomalies, included limit text, metrics).\n- Replaced someinnerHTMLusage with DOM-safetextContentwhere applicable.\n- Fixed linter warnings insrc/lib/usageHistory.tsand removed unnecessary async/await inarchiveOldDatato avoidrequire-awaitESLint rule.\n\nValidation:\n-npm run lintpasses.\n-npm testall suites pass (53 tests).\n\nNotes:\n- This partially addresses CodeQL findings by escaping values before adding them to HTML. If you prefer, we can take a more defensive route and replace templates with DOM construction entirely (safer but larger diff).\n- If you’d like me to further harden webview templates, I can convert the remaininginnerHTMLblocks to DOM assumptions usingcreateElement+textContentinstead of HTML templates.\n