-
Notifications
You must be signed in to change notification settings - Fork 205
Add copy SVG to clipboard option on icon click #412
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
Changes from all commits
15cc82a
262c685
0da86a4
2d1da31
ed35e25
6fb490e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| const iconsDir = path.join(__dirname, '../src/icons'); | ||
| const htmlPath = path.join(__dirname, '../dist/codicon.html'); | ||
|
|
||
| if (!fs.existsSync(htmlPath)) { | ||
| console.error('HTML file not found:', htmlPath); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| // Read all SVG files and build a data object | ||
| const svgData = {}; | ||
| const svgFiles = fs.readdirSync(iconsDir).filter(file => file.endsWith('.svg')); | ||
|
|
||
| for (const file of svgFiles) { | ||
| const name = path.basename(file, '.svg'); | ||
| const content = fs.readFileSync(path.join(iconsDir, file), 'utf8'); | ||
| svgData[name] = content; | ||
| } | ||
|
|
||
| let html = fs.readFileSync(htmlPath, 'utf8'); | ||
|
|
||
| if (html.includes('// SVG_DATA_PLACEHOLDER')) { | ||
| // Use regex to replace the initialization | ||
| html = html.replace(/let svgData = .*? \/\/ SVG_DATA_PLACEHOLDER/, `let svgData = ${JSON.stringify(svgData)}; // SVG_DATA_PLACEHOLDER`); | ||
| fs.writeFileSync(htmlPath, html); | ||
| console.log(`SVG data embedded into HTML (${Object.keys(svgData).length} icons).`); | ||
| } else { | ||
| console.warn('SVG data placeholder not found in HTML.'); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -117,10 +117,6 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| margin: 8px; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .icon:hover { | |
| cursor: pointer; | |
| } |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copy buttons are placed inside the .icon div but outside any semantic grouping. Consider wrapping the buttons in a container with role="group" and aria-label="Copy actions" to provide better context for screen reader users about what these buttons do in relation to the icon.
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The notification message uses the icon name directly in innerHTML without sanitization. While icon names are likely controlled data from the build process, if there's any possibility of user-controlled data in icon names, this could be an XSS vulnerability. Consider using textContent instead of innerHTML for displaying the icon name.
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
DOM text
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
In general, the fix is to avoid interpreting untrusted strings as HTML. Instead of assigning potentially tainted text to element.innerHTML, use element.textContent (or an equivalent safe text API) so that any HTML meta-characters are treated as plain text rather than markup.
Concretely, in src/template/preview.hbs, the function copyToClipboard uses notificationText.innerHTML = displayText; in three places (success in the modern clipboard path, success in the fallback, and in the SVG fetch error path). We should change all of these assignments to use notificationText.textContent = displayText;. This preserves the existing behavior of displaying the same text to the user while preventing any embedded HTML inside displayText (or the literal 'SVG not available') from being interpreted as markup. No additional methods or imports are needed; textContent is a standard DOM property. This single change addresses both reported alert variants because both ultimately use displayText in the same sink.
-
Copy modified line R632 -
Copy modified line R640 -
Copy modified line R648 -
Copy modified line R681
| @@ -629,7 +629,7 @@ | ||
| function copyToClipboard(text, displayText) { | ||
| if (navigator.clipboard && navigator.clipboard.writeText) { | ||
| navigator.clipboard.writeText(text).then(function() { | ||
| notificationText.innerHTML = displayText; | ||
| notificationText.textContent = displayText; | ||
| animateNotification(); | ||
| }).catch(function() { | ||
| // Fallback to old method | ||
| @@ -637,7 +637,7 @@ | ||
| copier.select(); | ||
| copier.setSelectionRange(0, 99999); | ||
| document.execCommand('copy'); | ||
| notificationText.innerHTML = displayText; | ||
| notificationText.textContent = displayText; | ||
| animateNotification(); | ||
| }); | ||
| } else { | ||
| @@ -645,7 +645,7 @@ | ||
| copier.select(); | ||
| copier.setSelectionRange(0, 99999); | ||
| document.execCommand('copy'); | ||
| notificationText.innerHTML = displayText; | ||
| notificationText.textContent = displayText; | ||
| animateNotification(); | ||
| } | ||
| } | ||
| @@ -678,7 +678,7 @@ | ||
| }) | ||
| .catch(error => { | ||
| console.warn('Could not fetch SVG for ' + iconName, error); | ||
| notificationText.innerHTML = 'SVG not available'; | ||
| notificationText.textContent = 'SVG not available'; | ||
| animateNotification(); | ||
| }); | ||
| } |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
DOM text
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
General approach: Avoid interpreting untrusted text as HTML. Instead of assigning displayText (which may come from data-name) to notificationText.innerHTML, assign it to notificationText.textContent so that any meta-characters are treated as literal text rather than markup. This preserves the visual behavior (showing a notification with the icon name or message) while eliminating the XSS sink.
Detailed best fix: In copyToClipboard, all three assignments to notificationText.innerHTML = displayText; should be changed to notificationText.textContent = displayText;. Likewise, in the SVG fetch catch handler, notificationText.innerHTML = 'SVG not available'; should be changed to notificationText.textContent = 'SVG not available';. These are all in src/template/preview.hbs around lines 631–641 and 679–682. No additional functions or imports are needed; textContent is a standard DOM property and behaves the same across modern browsers. This change does not alter the logic except to prevent HTML parsing of the message text.
-
Copy modified line R632 -
Copy modified line R640 -
Copy modified line R648 -
Copy modified line R681
| @@ -629,7 +629,7 @@ | ||
| function copyToClipboard(text, displayText) { | ||
| if (navigator.clipboard && navigator.clipboard.writeText) { | ||
| navigator.clipboard.writeText(text).then(function() { | ||
| notificationText.innerHTML = displayText; | ||
| notificationText.textContent = displayText; | ||
| animateNotification(); | ||
| }).catch(function() { | ||
| // Fallback to old method | ||
| @@ -637,7 +637,7 @@ | ||
| copier.select(); | ||
| copier.setSelectionRange(0, 99999); | ||
| document.execCommand('copy'); | ||
| notificationText.innerHTML = displayText; | ||
| notificationText.textContent = displayText; | ||
| animateNotification(); | ||
| }); | ||
| } else { | ||
| @@ -645,7 +645,7 @@ | ||
| copier.select(); | ||
| copier.setSelectionRange(0, 99999); | ||
| document.execCommand('copy'); | ||
| notificationText.innerHTML = displayText; | ||
| notificationText.textContent = displayText; | ||
| animateNotification(); | ||
| } | ||
| } | ||
| @@ -678,7 +678,7 @@ | ||
| }) | ||
| .catch(error => { | ||
| console.warn('Could not fetch SVG for ' + iconName, error); | ||
| notificationText.innerHTML = 'SVG not available'; | ||
| notificationText.textContent = 'SVG not available'; | ||
| animateNotification(); | ||
| }); | ||
| } |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
DOM text
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
In general, to fix “DOM text reinterpreted as HTML” issues, avoid passing untrusted strings to APIs that interpret them as HTML (innerHTML, outerHTML, jQuery’s html(), etc.). Instead, insert them as plain text using textContent, innerText, or appropriate escaping, so that any meta-characters are rendered literally rather than executed.
For this specific code, the displayText argument to copyToClipboard is derived from data-name and then used for user-visible notifications only. There is no requirement here to support HTML formatting in the notification; it should just display plain text like the icon name or “SVG not available”. Therefore, the best fix is to replace notificationText.innerHTML = displayText; with notificationText.textContent = displayText; in all places where displayText (or a string built from it) is shown. This preserves all current behavior (user still sees the same message, clipboard functionality is unchanged), but ensures that any special characters in data-name are not interpreted as HTML.
Concretely, in src/template/preview.hbs:
- In the
copyToClipboardfunction:- On the success path of
navigator.clipboard.writeText, changenotificationText.innerHTML = displayText;tonotificationText.textContent = displayText;. - On the catch (fallback) path, change the second
notificationText.innerHTML = displayText;similarly. - In the branch for environments without
navigator.clipboard, changenotificationText.innerHTML = displayText;likewise.
- On the success path of
- In the
fetcherror handler in the SVG copy code, changenotificationText.innerHTML = 'SVG not available';tonotificationText.textContent = 'SVG not available';.
No new methods, imports, or definitions are required; textContent is a standard DOM property.
-
Copy modified line R632 -
Copy modified line R640 -
Copy modified line R648 -
Copy modified line R681
| @@ -629,7 +629,7 @@ | ||
| function copyToClipboard(text, displayText) { | ||
| if (navigator.clipboard && navigator.clipboard.writeText) { | ||
| navigator.clipboard.writeText(text).then(function() { | ||
| notificationText.innerHTML = displayText; | ||
| notificationText.textContent = displayText; | ||
| animateNotification(); | ||
| }).catch(function() { | ||
| // Fallback to old method | ||
| @@ -637,7 +637,7 @@ | ||
| copier.select(); | ||
| copier.setSelectionRange(0, 99999); | ||
| document.execCommand('copy'); | ||
| notificationText.innerHTML = displayText; | ||
| notificationText.textContent = displayText; | ||
| animateNotification(); | ||
| }); | ||
| } else { | ||
| @@ -645,7 +645,7 @@ | ||
| copier.select(); | ||
| copier.setSelectionRange(0, 99999); | ||
| document.execCommand('copy'); | ||
| notificationText.innerHTML = displayText; | ||
| notificationText.textContent = displayText; | ||
| animateNotification(); | ||
| } | ||
| } | ||
| @@ -678,7 +678,7 @@ | ||
| }) | ||
| .catch(error => { | ||
| console.warn('Could not fetch SVG for ' + iconName, error); | ||
| notificationText.innerHTML = 'SVG not available'; | ||
| notificationText.textContent = 'SVG not available'; | ||
| animateNotification(); | ||
| }); | ||
| } |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fetch fallback for SVG data uses a relative path 'icons/' + iconName + '.svg' but there's no verification that this path is correct or that the icons directory is available at this location in the deployed site. Consider documenting where these files should be located or ensuring the build process copies them to the expected location.
This issue also appears in the following locations of the same file:
- line 670
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code doesn't check if the querySelector calls return null before assigning onclick handlers. If the template structure changes or an icon doesn't have the expected buttons, this will throw an error. Add null checks before assigning the onclick handlers.
| copyNameBtn.onclick = function(e){ | |
| e.stopPropagation(); | |
| let iconName = this.closest('.icon').getAttribute('data-name'); | |
| copyToClipboard(iconName, iconName); | |
| }; | |
| // Copy SVG button | |
| let copySvgBtn = icon.querySelector('.copy-svg-btn'); | |
| copySvgBtn.onclick = function(e){ | |
| e.stopPropagation(); | |
| let iconName = this.closest('.icon').getAttribute('data-name'); | |
| if (svgData[iconName]) { | |
| copyToClipboard(svgData[iconName], iconName + ' (SVG)'); | |
| } else { | |
| // Fallback: try to fetch SVG if not in embedded data | |
| fetch('icons/' + iconName + '.svg') | |
| .then(response => response.text()) | |
| .then(svg => { | |
| copyToClipboard(svg, iconName + ' (SVG)'); | |
| }) | |
| .catch(error => { | |
| console.warn('Could not fetch SVG for ' + iconName, error); | |
| notificationText.innerHTML = 'SVG not available'; | |
| animateNotification(); | |
| }); | |
| } | |
| }; | |
| if (copyNameBtn) { | |
| copyNameBtn.onclick = function(e){ | |
| e.stopPropagation(); | |
| let iconName = this.closest('.icon').getAttribute('data-name'); | |
| copyToClipboard(iconName, iconName); | |
| }; | |
| } | |
| // Copy SVG button | |
| let copySvgBtn = icon.querySelector('.copy-svg-btn'); | |
| if (copySvgBtn) { | |
| copySvgBtn.onclick = function(e){ | |
| e.stopPropagation(); | |
| let iconName = this.closest('.icon').getAttribute('data-name'); | |
| if (svgData[iconName]) { | |
| copyToClipboard(svgData[iconName], iconName + ' (SVG)'); | |
| } else { | |
| // Fallback: try to fetch SVG if not in embedded data | |
| fetch('icons/' + iconName + '.svg') | |
| .then(response => response.text()) | |
| .then(svg => { | |
| copyToClipboard(svg, iconName + ' (SVG)'); | |
| }) | |
| .catch(error => { | |
| console.warn('Could not fetch SVG for ' + iconName, error); | |
| notificationText.innerHTML = 'SVG not available'; | |
| animateNotification(); | |
| }); | |
| } | |
| }; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script checks if the HTML file exists but doesn't verify that the icons directory exists before attempting to read it. This could result in a cryptic error if the directory is missing. Consider adding a check similar to the HTML file verification.