Fix: Scrum report injection on Outlook and Yahoo in Firefox (#135)#279
Closed
saurabh24thakur wants to merge 3 commits intofossasia:mainfrom
Closed
Fix: Scrum report injection on Outlook and Yahoo in Firefox (#135)#279saurabh24thakur wants to merge 3 commits intofossasia:mainfrom
saurabh24thakur wants to merge 3 commits intofossasia:mainfrom
Conversation
Contributor
Reviewer's GuideAdjusts the email content injection logic for Outlook and Yahoo to handle Firefox-specific behavior using execCommand-based insertion and conditional event dispatching so Scrum reports paste correctly across clients. Sequence diagram for Firefox-aware email content injectionsequenceDiagram
participant EmailClientAdapter
participant Browser
participant OutlookEditor
participant YahooEditor
EmailClientAdapter->>Browser: detectClient()
Browser-->>EmailClientAdapter: clientType
EmailClientAdapter->>EmailClientAdapter: config = clientConfigs[clientType]
EmailClientAdapter->>Browser: navigator.userAgent
Browser-->>EmailClientAdapter: userAgent
EmailClientAdapter->>EmailClientAdapter: isFirefox = userAgent.includes(firefox)
alt injectMethod is focusAndPaste (Outlook)
EmailClientAdapter->>OutlookEditor: focus()
alt isFirefox
EmailClientAdapter->>Browser: execCommand(insertHTML, false, content)
alt execCommand fails
EmailClientAdapter->>OutlookEditor: innerHTML = content
end
else not Firefox
EmailClientAdapter->>OutlookEditor: innerHTML = content
end
EmailClientAdapter->>OutlookEditor: dispatchElementEvents([input, change], true)
else injectMethod is setContent (Yahoo)
alt isFirefox
EmailClientAdapter->>YahooEditor: focus()
EmailClientAdapter->>Browser: execCommand(insertHTML, false, content)
alt execCommand fails
EmailClientAdapter->>YahooEditor: innerHTML = content
end
EmailClientAdapter->>YahooEditor: dispatchElementEvents([input, change])
else not Firefox
EmailClientAdapter->>YahooEditor: innerHTML = content
EmailClientAdapter->>YahooEditor: focus()
EmailClientAdapter->>Browser: getSelection()
Browser-->>EmailClientAdapter: selection
EmailClientAdapter->>Browser: createRange()
Browser-->>EmailClientAdapter: range
EmailClientAdapter->>Browser: range.selectNodeContents(element)
EmailClientAdapter->>Browser: selection.removeAllRanges()
EmailClientAdapter->>Browser: selection.addRange(range)
EmailClientAdapter->>YahooEditor: dispatchElementEvents([input, change])
end
end
Updated class diagram for EmailClientAdapter Firefox injection logicclassDiagram
class EmailClientAdapter {
+clientConfigs
+detectClient() string
+injectContent(element, content) void
+dispatchElementEvents(element, eventTypes, bubbles) void
}
class BrowserEnvironment {
+navigatorUserAgent string
+execCommand(command, ui, value) bool
+getSelection() Selection
+createRange() Range
}
class Selection {
+removeAllRanges() void
+addRange(range) void
}
class Range {
+selectNodeContents(element) void
}
EmailClientAdapter --> BrowserEnvironment : uses
EmailClientAdapter ..> Selection : uses
EmailClientAdapter ..> Range : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
javascript
![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 security issues, 1 other issue, and left some high level feedback:
Security issues:
- User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
element.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
General comments:
- The Firefox-specific insertion logic is duplicated between the
focusAndPasteandsetContentbranches; consider extracting a shared helper forinsertHTML/innerHTMLfallback to keep behavior consistent and easier to maintain. - In the Yahoo
setContentpath, the non-Firefox branch forces selection updates while the Firefox branch does not; if this difference is intentional, a short comment would help future readers understand why selection handling is skipped on Firefox.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Firefox-specific insertion logic is duplicated between the `focusAndPaste` and `setContent` branches; consider extracting a shared helper for `insertHTML`/`innerHTML` fallback to keep behavior consistent and easier to maintain.
- In the Yahoo `setContent` path, the non-Firefox branch forces selection updates while the Firefox branch does not; if this difference is intentional, a short comment would help future readers understand why selection handling is skipped on Firefox.
## Individual Comments
### Comment 1
<location> `src/scripts/emailClientAdapter.js:182` </location>
<code_context>
+ if (isFirefox) {
</code_context>
<issue_to_address>
**question (bug_risk):** Firefox `setContent` branch no longer forces selection/range; confirm this won’t break Yahoo-specific behavior.
Previously this path relied on forcing a selection/range update so Yahoo’s editor would register the content change. The new Firefox-specific path only fires `input`/`change` events, which may change behavior for Yahoo in Firefox versus other browsers. Can you confirm this remains correct on Yahoo in Firefox? If so, consider adding a brief note (PR or code comment) to document that this divergence from other browsers is intentional.
</issue_to_address>
### Comment 2
<location> `src/scripts/emailClientAdapter.js:184` </location>
<code_context>
element.innerHTML = content;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 3
<location> `src/scripts/emailClientAdapter.js:184` </location>
<code_context>
element.innerHTML = content;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `element.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
|
@saurabh24thakur Please add screenshots/screen recordings |
Author
https://drive.google.com/file/d/1feLIyQIth2ehxsk9U_rm4__ieUrv0WBz/view?usp=sharing @vedansh-5 here is the recording... |
Member
|
Uploading the video here for documentation purposes. Please avoid Google drive links here. We prefer to keep documentation on one platform. Thanks scrum-helper.illustration.mp4 |
Member
|
Following FOSSASIA best practices, changes should be proposed via an issue before submitting a pull request. The issue should clearly describe the problem and the intended approach. Once the issue exists, the pull request should reference it using the standard closing syntax, for example: “Resolves #123”. This ensures proper tracking, discussion, and review of the change. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes issues with Scrum report content injection into Outlook and Yahoo email editors when using Firefox. The changes refactor HTML content insertion to use browser-specific approaches, particularly leveraging execCommand('insertHTML') for Firefox and maintaining DOM-based insertion for other browsers.
Changes:
- Introduced helper methods
_insertHtmlContentand_safeSetHTMLfor safer HTML injection - Added Firefox-specific handling using
execCommand('insertHTML') - Updated Outlook and Yahoo email editor injection logic with browser-specific paths
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Note: Content is assumed to be trusted (generated by extension). | ||
| */ | ||
| _insertHtmlContent(element, content) { | ||
| const isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1; |
There was a problem hiding this comment.
Browser detection using user agent strings (navigator.userAgent) is declared twice within the same method - once at line 158 for the _insertHtmlContent function, and then again at line 201 inside the setContent case. Consider extracting this check into a method-level or class-level variable to avoid redundant computation and improve maintainability.
Comment on lines
+201
to
+219
| const isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1; | ||
|
|
||
| if (isFirefox) { | ||
| // Firefox: execCommand handles selection updates automatically. | ||
| this._insertHtmlContent(element, content); | ||
| this.dispatchElementEvents(element, ['input', 'change']); | ||
| } else { | ||
| // Chrome/Edge: Direct assignment requires manual selection update | ||
| // to ensure the editor recognizes the change. | ||
| this._safeSetHTML(element, content); | ||
|
|
||
| const selection = window.getSelection(); | ||
| const range = document.createRange(); | ||
| range.selectNodeContents(element); | ||
| selection.removeAllRanges(); | ||
| selection.addRange(range); | ||
|
|
||
| this.dispatchElementEvents(element, ['input', 'change']); | ||
| } |
There was a problem hiding this comment.
Browser detection is duplicated in the setContent case at line 201, even though _insertHtmlContent already contains this logic. The browser detection should be removed from line 201 and the method should rely on _insertHtmlContent to handle the Firefox-specific logic consistently.
Suggested change
| const isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1; | |
| if (isFirefox) { | |
| // Firefox: execCommand handles selection updates automatically. | |
| this._insertHtmlContent(element, content); | |
| this.dispatchElementEvents(element, ['input', 'change']); | |
| } else { | |
| // Chrome/Edge: Direct assignment requires manual selection update | |
| // to ensure the editor recognizes the change. | |
| this._safeSetHTML(element, content); | |
| const selection = window.getSelection(); | |
| const range = document.createRange(); | |
| range.selectNodeContents(element); | |
| selection.removeAllRanges(); | |
| selection.addRange(range); | |
| this.dispatchElementEvents(element, ['input', 'change']); | |
| } | |
| this._insertHtmlContent(element, content); | |
| this.dispatchElementEvents(element, ['input', 'change']); |
Comment on lines
+159
to
+164
|
|
||
| if (isFirefox) { | ||
| // Firefox often blocks direct innerHTML access in some contexts or doesn't trigger editor updates. | ||
| // execCommand('insertHTML') is the standard way to insert content into a contentEditable. | ||
| // It also handles cursor placement and selection updates automatically. | ||
| if (!document.execCommand('insertHTML', false, content)) { |
There was a problem hiding this comment.
The execCommand API at line 164 is deprecated and may not be supported in future browser versions. While it addresses the current Firefox issue, consider adding a fallback mechanism or a comment acknowledging this deprecation for future maintainability.
Suggested change
| if (isFirefox) { | |
| // Firefox often blocks direct innerHTML access in some contexts or doesn't trigger editor updates. | |
| // execCommand('insertHTML') is the standard way to insert content into a contentEditable. | |
| // It also handles cursor placement and selection updates automatically. | |
| if (!document.execCommand('insertHTML', false, content)) { | |
| if (isFirefox) { | |
| // Firefox often blocks direct innerHTML access in some contexts or doesn't trigger editor updates. | |
| // Historically, execCommand('insertHTML') has been the standard way to insert content into a | |
| // contentEditable while preserving cursor placement and selection updates. | |
| // NOTE: document.execCommand is deprecated and may be removed in future browsers. | |
| // We therefore feature-detect its availability and fall back to _safeSetHTML if it is unavailable | |
| // or fails for any reason, to keep this code forwards-compatible. | |
| const hasExecCommand = typeof document.execCommand === 'function'; | |
| const supportsInsertHtml = | |
| typeof document.queryCommandSupported === 'function' | |
| ? document.queryCommandSupported('insertHTML') | |
| : true; // assume true if we cannot detect support explicitly | |
| if (hasExecCommand && supportsInsertHtml) { | |
| try { | |
| if (!document.execCommand('insertHTML', false, content)) { | |
| this._safeSetHTML(element, content); | |
| } | |
| } catch (e) { | |
| this._safeSetHTML(element, content); | |
| } | |
| } else { |
Comment on lines
+201
to
+219
| const isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1; | ||
|
|
||
| if (isFirefox) { | ||
| // Firefox: execCommand handles selection updates automatically. | ||
| this._insertHtmlContent(element, content); | ||
| this.dispatchElementEvents(element, ['input', 'change']); | ||
| } else { | ||
| // Chrome/Edge: Direct assignment requires manual selection update | ||
| // to ensure the editor recognizes the change. | ||
| this._safeSetHTML(element, content); | ||
|
|
||
| const selection = window.getSelection(); | ||
| const range = document.createRange(); | ||
| range.selectNodeContents(element); | ||
| selection.removeAllRanges(); | ||
| selection.addRange(range); | ||
|
|
||
| this.dispatchElementEvents(element, ['input', 'change']); | ||
| } |
There was a problem hiding this comment.
The logic in the setContent case (lines 203-219) duplicates the browser-specific behavior already handled by _insertHtmlContent. The Firefox branch calls _insertHtmlContent which will perform another browser check, while the Chrome/Edge branch could simply call _safeSetHTML and handle selection separately. This creates inconsistent logic flow.
Suggested change
| const isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1; | |
| if (isFirefox) { | |
| // Firefox: execCommand handles selection updates automatically. | |
| this._insertHtmlContent(element, content); | |
| this.dispatchElementEvents(element, ['input', 'change']); | |
| } else { | |
| // Chrome/Edge: Direct assignment requires manual selection update | |
| // to ensure the editor recognizes the change. | |
| this._safeSetHTML(element, content); | |
| const selection = window.getSelection(); | |
| const range = document.createRange(); | |
| range.selectNodeContents(element); | |
| selection.removeAllRanges(); | |
| selection.addRange(range); | |
| this.dispatchElementEvents(element, ['input', 'change']); | |
| } | |
| this._insertHtmlContent(element, content); | |
| this.dispatchElementEvents(element, ['input', 'change']); |
Member
|
@saurabh24thakur Thanks for your contribution, Please address copilots' concerns |
Member
|
Contributor has been inactive for more than 2 weeks. I am closing this PR to let others work on the issue. |
Author
ok i will |
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 # (Use "Fixes", "Closes", or "Resolves" for automatic closing)
📝 Summary of Changes
📸 Screenshots / Demo (if UI-related)
Add screenshots, video, or link to deployed preview if applicable
✅ Checklist
👀 Reviewer Notes
Add any special notes for the reviewer here
Summary by Sourcery
Improve HTML content injection into email editors for Outlook and Yahoo, with Firefox-specific handling.
Bug Fixes: