Replaced unsafe innerHTML with safe DOM APIs in popup.js#526
Open
diyasharma12 wants to merge 5 commits intofossasia:mainfrom
Open
Replaced unsafe innerHTML with safe DOM APIs in popup.js#526diyasharma12 wants to merge 5 commits intofossasia:mainfrom
diyasharma12 wants to merge 5 commits intofossasia:mainfrom
Conversation
Contributor
Reviewer's GuideRefactors popup.js to eliminate unsafe innerHTML usage by introducing reusable DOM helper functions and replacing string-based HTML construction with explicit DOM node creation for buttons, repo dropdown, repo tags, and scrum report UI, while also adding a one-time reportConfig migration call on DOMContentLoaded. Sequence diagram for refreshed cache and scrum report update in popup.jssequenceDiagram
actor User
participant PopupUI
participant RefreshCacheButton
participant BrowserStorage as BrowserAPI
participant ScrumReport
participant RepoState as RepoUIState
User ->> RefreshCacheButton: click
RefreshCacheButton ->> RefreshCacheButton: store originalChildren
RefreshCacheButton ->> RefreshCacheButton: add class loading
RefreshCacheButton ->> RefreshCacheButton: setButtonIconWithSpanText(spinner, refreshingButton)
RefreshCacheButton ->> RefreshCacheButton: disabled = true
RefreshCacheButton ->> BrowserStorage: clear or refresh cache
BrowserStorage -->> RefreshCacheButton: resolve or reject
alt cache clear success
RefreshCacheButton ->> RepoState: reset availableRepos, selectedRepos
RefreshCacheButton ->> RepoState: updateRepoDisplay()
RefreshCacheButton ->> ScrumReport: replaceChildren(p cacheClearedMessage)
RefreshCacheButton ->> RefreshCacheButton: setButtonIconWithSpanText(check, cacheClearedButton)
RefreshCacheButton ->> RefreshCacheButton: remove class loading
RefreshCacheButton ->> RefreshCacheButton: schedule timeout 2s
PopupUI ->> RefreshCacheButton: timeout fires
RefreshCacheButton ->> RefreshCacheButton: restore originalChildren
RefreshCacheButton ->> RefreshCacheButton: disabled = false
else cache clear failure
RefreshCacheButton ->> RefreshCacheButton: setButtonIconWithSpanText(exclamation, cacheClearFailed)
RefreshCacheButton ->> RefreshCacheButton: remove class loading
RefreshCacheButton ->> RefreshCacheButton: schedule timeout 3s
PopupUI ->> RefreshCacheButton: timeout fires
RefreshCacheButton ->> RefreshCacheButton: restore originalChildren
RefreshCacheButton ->> RefreshCacheButton: disabled = false
end
Class diagram for new DOM helper functions in popup.jsclassDiagram
class PopupDOMHelpers {
+clearElementChildren(element)
+setButtonIconAndText(button, iconClasses, text)
+setButtonIconWithSpanText(button, iconClasses, text)
}
class RepoDropdownUI {
+filterAndDisplayRepos(query)
}
class RepoTagsUI {
+updateRepoDisplay()
}
class PlatformDropdownUI {
+setPlatformDropdown(value)
}
class RefreshCacheButtonUI {
+onClick()
}
class ScrumReportUI {
+showCacheClearedMessage()
}
RepoDropdownUI ..> PopupDOMHelpers : uses
RepoTagsUI ..> PopupDOMHelpers : uses
PlatformDropdownUI ..> PopupDOMHelpers : uses
RefreshCacheButtonUI ..> PopupDOMHelpers : uses
ScrumReportUI ..> PopupDOMHelpers : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The two helpers
setButtonIconAndTextandsetButtonIconWithSpanTextare very similar; consider consolidating them (e.g., with an optional wrapper element for the text) to reduce duplication and keep button updates consistent. - In the
refreshCacheclick handler,originalChildrenare cloned twice (once when captured and again when restoring); you could store aDocumentFragmentor reuse the original nodes to avoid the extra cloning and make the restore logic simpler.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The two helpers `setButtonIconAndText` and `setButtonIconWithSpanText` are very similar; consider consolidating them (e.g., with an optional wrapper element for the text) to reduce duplication and keep button updates consistent.
- In the `refreshCache` click handler, `originalChildren` are cloned twice (once when captured and again when restoring); you could store a `DocumentFragment` or reuse the original nodes to avoid the extra cloning and make the restore logic simpler.
## Individual Comments
### Comment 1
<location path="src/scripts/popup.js" line_range="97-102" />
<code_context>
+ }
+}
+
+function setButtonIconAndText(button, iconClasses, text) {
+ if (!button) return;
+ const icon = document.createElement('i');
+ icon.className = iconClasses;
+ const label = document.createTextNode(` ${text || ''}`);
+ button.replaceChildren(icon, label);
+}
+
</code_context>
<issue_to_address>
**suggestion:** Avoid treating valid falsy text values (like '0') as empty in the shared button helpers.
`setButtonIconAndText` (and `setButtonIconWithSpanText`) use `text || ''`, which converts valid falsy values like `0` or `false` to an empty string. If localized strings or counts can be `'0'`, they’ll disappear in the UI. Use nullish coalescing (`text ?? ''`) instead so only `null`/`undefined` are treated as empty.
Suggested implementation:
```javascript
function setButtonIconAndText(button, iconClasses, text) {
if (!button) return;
const icon = document.createElement('i');
icon.className = iconClasses;
const label = document.createTextNode(` ${text ?? ''}`);
button.replaceChildren(icon, label);
}
```
There’s another helper mentioned in your comment: `setButtonIconWithSpanText`. To fully apply your suggestion, update that function similarly by replacing any `text || ''` usage with `text ?? ''` so that only `null`/`undefined` result in an empty string, preserving values like `0` or `false`.
</issue_to_address>
### Comment 2
<location path="src/scripts/popup.js" line_range="2022-2028" />
<code_context>
document.getElementById('refreshCache').addEventListener('click', async function () {
- const originalText = this.innerHTML;
+ const originalChildren = Array.from(this.childNodes).map((node) => node.cloneNode(true));
this.classList.add('loading');
</code_context>
<issue_to_address>
**issue (bug_risk):** Restoring button content via double-cloned children can drop existing event listeners and is more complex than necessary.
You can avoid this by snapshotting the existing nodes instead of cloning them twice, e.g.:
```js
const originalChildren = Array.from(this.childNodes);
// ... later
this.replaceChildren(...originalChildren);
```
This preserves the original nodes (and any attached listeners) and simplifies the implementation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to harden the extension popup UI (src/scripts/popup.js) against XSS by replacing innerHTML-based rendering of API/user-controlled content with safer DOM construction patterns.
Changes:
- Introduces helper utilities (
clearElementChildren,setButtonIconAndText,setButtonIconWithSpanText) to standardize safe DOM updates. - Refactors repository dropdown/tag rendering and several button/report UI states to use
textContent/createElement/replaceChildreninstead ofinnerHTML. - Adds a
ReportConfigStoragelegacy migration call during popup initialization.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
mariobehling
requested changes
Apr 11, 2026
Member
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Please address AI reviews or comment if not relevant.
Contributor
Author
|
I've completed a full security audit. All mentioned innerHTML usage is removed. The PR now meets all security acceptance criteria. |
4 tasks
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.
📌 Fixes
Fixes #524
📝 Summary of Changes
This PR addresses 18 blocking security findings identified by the sourcery security scan. The original implementation used innerHTML to render API data and user-controlled strings in src/scripts/popup.js, creating potential Cross-Site Scripting (XSS) vulnerabilities.
Changes Made
✅ Checklist
Summary by Sourcery
Harden popup UI rendering by replacing unsafe HTML injection with structured DOM updates and centralizing button/icon handling.
Bug Fixes:
Enhancements: