fix: DOMPurify hook memory leak in MarkdownText#38834
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
…omponent Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
| const registerDOMPurifyHooks = () => { | ||
| if (hooksRegistered) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
@copilot is this really necessary? why not just register this function is not going to be run twice anyway
There was a problem hiding this comment.
You're right - the wrapper function and guard were unnecessary since the code only executes once at module level. Simplified in 63b8a94.
Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
| // For internal links, add a translated title with the relative path | ||
| // Example: for href "https://my-server.rocket.chat/channel/general" the title would be "Go to #general" | ||
| // Using i18next directly ensures we always get the current translation | ||
| node.setAttribute('title', `${i18next.t('Go_to_href', { href: href.replace(getBaseURI(), '') })}`); |
There was a problem hiding this comment.
@copilot I was thinking whether we could somehow place a random token here and perform the translation inside the component. That way, we can still use the contextualized t.
There was a problem hiding this comment.
Implemented token-based approach in 1a6e58f. The hook now places __INTERNAL_LINK_TITLE__ token for internal links, and the component performs string replacement using contextualized t() after sanitization. This keeps the hook at module level (no leak) while using React's translation context.
…function Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
|
|
||
| // Special token used to mark internal links that need translation | ||
| // This allows the hook to stay at module level while still using contextualized translations | ||
| const INTERNAL_LINK_TOKEN = '__INTERNAL_LINK_TITLE__'; |
There was a problem hiding this comment.
@copilot can we add some runtime generation here? to prevent any enumeration?
There was a problem hiding this comment.
Added runtime generation in 37a094a. The token is now generated using Math.random().toString(36) at module load time, creating a unique unpredictable string (e.g., __INTERNAL_LINK_TITLE_k3j9f2h8d__) that prevents enumeration attacks.
Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
…hook registration is handled properly
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38834 +/- ##
===========================================
+ Coverage 70.53% 70.58% +0.04%
===========================================
Files 3183 3184 +1
Lines 112485 112547 +62
Branches 20407 20405 -2
===========================================
+ Hits 79338 79437 +99
+ Misses 31087 31047 -40
- Partials 2060 2063 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/jira ARCH-2021 |
1 similar comment
|
/jira ARCH-2021 |
Summary
Fixed critical memory leak in MarkdownText.tsx by moving DOMPurify hook registration from useMemo (component level) to module level. Hook now registers once globally instead of per-component instance, eliminating accumulation of duplicate hooks (100+ → 1) and reducing hook calls during normal usage (5850+ → ~58).
Implementation Details
The fix uses a token-based approach with runtime generation to maintain both module-level hook registration (preventing memory leaks) and React's contextualized translation system:
Math.random()to generate a unique, unpredictable token at runtime (e.g.,__INTERNAL_LINK_TITLE_k3j9f2h8d__)useTranslation()hookOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.