From 453e36bf6695b1a5732d77bed9409b711078fc7b Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Fri, 26 Jul 2024 16:41:34 -0300 Subject: [PATCH 01/10] check all the links even when there is no target set --- apps/meteor/client/components/MarkdownText.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/apps/meteor/client/components/MarkdownText.tsx b/apps/meteor/client/components/MarkdownText.tsx index da65fd3b1b48f..04a3fdd61dc16 100644 --- a/apps/meteor/client/components/MarkdownText.tsx +++ b/apps/meteor/client/components/MarkdownText.tsx @@ -132,7 +132,7 @@ const MarkdownText = ({ // Add a hook to make all external links open a new window dompurify.addHook('afterSanitizeAttributes', (node) => { - if (isElement(node) && 'target' in node) { + if (node.tagName.toLowerCase() === 'a') { const href = node.getAttribute('href') || ''; node.setAttribute('title', `${t('Go_to_href', { href: href.replace(getBaseURI(), '') })}`); @@ -157,6 +157,4 @@ const MarkdownText = ({ ) : null; }; -const isElement = (node: Node): node is Element => node.nodeType === Node.ELEMENT_NODE; - export default MarkdownText; From cbdb845b3597b9b593fb32beb5c9503b6891bbef Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Mon, 5 Aug 2024 12:37:12 -0300 Subject: [PATCH 02/10] Create serious-trees-whisper.md --- .changeset/serious-trees-whisper.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/serious-trees-whisper.md diff --git a/.changeset/serious-trees-whisper.md b/.changeset/serious-trees-whisper.md new file mode 100644 index 0000000000000..aeca4c1d11cc3 --- /dev/null +++ b/.changeset/serious-trees-whisper.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": patch +--- + +Fixed links on settings redirecting to the outside of the chat, now they will open on a new tab From 25ae74b6c17486a905c8aa399e3a26e1d7fa8f85 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Fri, 9 May 2025 12:06:44 -0300 Subject: [PATCH 03/10] refactor: improve link element detection in MarkdownText component --- apps/meteor/client/components/MarkdownText.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/meteor/client/components/MarkdownText.tsx b/apps/meteor/client/components/MarkdownText.tsx index 04a3fdd61dc16..ac487798a7de7 100644 --- a/apps/meteor/client/components/MarkdownText.tsx +++ b/apps/meteor/client/components/MarkdownText.tsx @@ -132,7 +132,7 @@ const MarkdownText = ({ // Add a hook to make all external links open a new window dompurify.addHook('afterSanitizeAttributes', (node) => { - if (node.tagName.toLowerCase() === 'a') { + if (isLinkElement(node)) { const href = node.getAttribute('href') || ''; node.setAttribute('title', `${t('Go_to_href', { href: href.replace(getBaseURI(), '') })}`); @@ -157,4 +157,7 @@ const MarkdownText = ({ ) : null; }; +const isElement = (node: Node): node is Element => node.nodeType === Node.ELEMENT_NODE; +const isLinkElement = (node: Node): node is HTMLAnchorElement => isElement(node) && node.tagName.toLowerCase() === 'a'; + export default MarkdownText; From 126ea8d47fa5e8a2f902b7787a42496ef067a741 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Fri, 9 May 2025 12:08:29 -0300 Subject: [PATCH 04/10] enable back DOMPurify on settings --- apps/meteor/client/views/admin/settings/Setting/Setting.tsx | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/apps/meteor/client/views/admin/settings/Setting/Setting.tsx b/apps/meteor/client/views/admin/settings/Setting/Setting.tsx index 04f4e8cf061e9..a33ca271e1d70 100644 --- a/apps/meteor/client/views/admin/settings/Setting/Setting.tsx +++ b/apps/meteor/client/views/admin/settings/Setting/Setting.tsx @@ -104,10 +104,7 @@ function Setting({ className = undefined, settingId, sectionChanged }: SettingPr const labelText = (i18n.exists(i18nLabel) && t(i18nLabel)) || (i18n.exists(_id) && t(_id)) || i18nLabel || _id; const hint = useMemo( - () => - i18nDescription && i18n.exists(i18nDescription) ? ( - - ) : undefined, + () => (i18nDescription && i18n.exists(i18nDescription) ? : undefined), [i18n, i18nDescription, t], ); From 48403dcca2c5c2cd567ca8f58411c58895c198b4 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Mon, 12 May 2025 16:33:47 -0300 Subject: [PATCH 05/10] refactor afterSanitizeAttributes --- .../meteor/client/components/MarkdownText.tsx | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/apps/meteor/client/components/MarkdownText.tsx b/apps/meteor/client/components/MarkdownText.tsx index ac487798a7de7..ebf751b2455a6 100644 --- a/apps/meteor/client/components/MarkdownText.tsx +++ b/apps/meteor/client/components/MarkdownText.tsx @@ -132,20 +132,23 @@ const MarkdownText = ({ // Add a hook to make all external links open a new window dompurify.addHook('afterSanitizeAttributes', (node) => { - if (isLinkElement(node)) { - const href = node.getAttribute('href') || ''; + if (!isLinkElement(node)) { + return; + } + + const href = node.getAttribute('href') || ''; - node.setAttribute('title', `${t('Go_to_href', { href: href.replace(getBaseURI(), '') })}`); + node.setAttribute('title', `${t('Go_to_href', { href: href.replace(getBaseURI(), '') })}`); + + if (isExternal(href)) { + node.setAttribute('target', '_blank'); node.setAttribute('rel', 'nofollow noopener noreferrer'); - if (isExternal(node.getAttribute('href') || '')) { - node.setAttribute('target', '_blank'); - node.setAttribute('title', href); - } + node.setAttribute('title', href); } }); return preserveHtml ? html : html && sanitizer(html, { ADD_ATTR: ['target'], ALLOWED_URI_REGEXP: getRegexp(schemes) }); - }, [preserveHtml, sanitizer, content, variant, markedOptions, parseEmoji, t]); + }, [preserveHtml, sanitizer, content, variant, markedOptions, parseEmoji, t, schemes]); return __html ? ( Date: Wed, 14 May 2025 19:44:00 -0300 Subject: [PATCH 06/10] fix tests --- .../client/components/MarkdownText.spec.tsx | 27 +++++++++++++---- .../meteor/client/components/MarkdownText.tsx | 30 +++++++++++++++---- 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/apps/meteor/client/components/MarkdownText.spec.tsx b/apps/meteor/client/components/MarkdownText.spec.tsx index 08671faec7069..16d87bb5f6d38 100644 --- a/apps/meteor/client/components/MarkdownText.spec.tsx +++ b/apps/meteor/client/components/MarkdownText.spec.tsx @@ -58,8 +58,17 @@ it('should render html elements as expected using default parser', async () => { expect(normalizedHtml).toContain('

Heading 3

'); expect(normalizedHtml).toContain('
  • List Item 1
  • List Item 2
  • List Item 3
  • List Item 4'); expect(normalizedHtml).toContain('
    1. List Item 1
    2. List Item 2
    3. List Item 3
    4. List Item 4'); - expect(normalizedHtml).toContain('Rocket.Chat'); + + expect(normalizedHtml).toContain('Rocket.Chat'); + + expect(normalizedHtml).toContain('href="mailto:gabriel.engel@rocket.chat"'); + expect(normalizedHtml).toContain('title="mailto:gabriel.engel@rocket.chat"'); expect(normalizedHtml).toContain('gabriel.engel@rocket.chat'); + expect(normalizedHtml).toContain('+55991999999'); expect(normalizedHtml).toContain('Inline code'); expect(normalizedHtml).toContain('
      const test = \'this is code\'
      '); @@ -87,10 +96,18 @@ it('should render html elements as expected using inline parser', async () => { expect(normalizedHtml).toContain('### Heading 3'); expect(normalizedHtml).toContain('Unordered List - List Item 1 - List Item 2 - List Item 3 - List Item 4'); expect(normalizedHtml).toContain('Ordered List 1. List Item 1 2. List Item 2 3. List Item 3 4. List Item 4'); - expect(normalizedHtml).toContain(`Rocket.Chat`); - expect(normalizedHtml).toContain( - `gabriel.engel@rocket.chat`, - ); + + expect(normalizedHtml).toContain('Rocket.Chat'); + + expect(normalizedHtml).toContain('href="mailto:gabriel.engel@rocket.chat"'); + expect(normalizedHtml).toContain('title="mailto:gabriel.engel@rocket.chat"'); + expect(normalizedHtml).toContain('rel="nofollow noopener noreferrer"'); + expect(normalizedHtml).toContain('target="_blank"'); + expect(normalizedHtml).toContain('+55991999999'); expect(normalizedHtml).toContain('Inline code'); expect(normalizedHtml).toContain(`typescript const test = 'this is code'`); diff --git a/apps/meteor/client/components/MarkdownText.tsx b/apps/meteor/client/components/MarkdownText.tsx index ebf751b2455a6..6a140f80a4909 100644 --- a/apps/meteor/client/components/MarkdownText.tsx +++ b/apps/meteor/client/components/MarkdownText.tsx @@ -32,8 +32,9 @@ const walkTokens = (token: marked.Token) => { marked.use({ walkTokens }); -const linkMarked = (href: string | null, _title: string | null, text: string): string => - `${text} `; +const linkMarked = (href: string | null, _title: string | null, text: string): string => { + return `${text}`; +}; const paragraphMarked = (text: string): string => text; const brMarked = (): string => ' '; const listItemMarked = (text: string): string => { @@ -137,13 +138,30 @@ const MarkdownText = ({ } const href = node.getAttribute('href') || ''; + const isExternalLink = isExternal(href); + const isMailto = href.startsWith('mailto:'); - node.setAttribute('title', `${t('Go_to_href', { href: href.replace(getBaseURI(), '') })}`); - - if (isExternal(href)) { - node.setAttribute('target', '_blank'); + // Set appropriate attributes based on link type + if (isExternalLink || isMailto) { node.setAttribute('rel', 'nofollow noopener noreferrer'); + // Enforcing external links to open in new tabs is critical to assure users never navigate away from the chat + // This attribute must be preserved to guarantee users maintain their chat context + node.setAttribute('target', '_blank'); + } + + // Set appropriate title based on link type + if (isExternalLink) { + // For external links, set an empty title to prevent tooltips + // This reduces visual clutter and lets users see the URL in the browser's status bar instead + node.setAttribute('title', ''); + } else if (isMailto) { + // For mailto links, use the email address as the title for better user experience + // Example: for href "mailto:user@example.com" the title would be "mailto:user@example.com" node.setAttribute('title', href); + } else { + // 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" + node.setAttribute('title', `${t('Go_to_href', { href: href.replace(getBaseURI(), '') })}`); } }); From 5ff54e6eff61f7aabcc04b6d604abbbacea45fd0 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 14 May 2025 20:23:33 -0300 Subject: [PATCH 07/10] fix order --- apps/meteor/client/components/MarkdownText.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/apps/meteor/client/components/MarkdownText.tsx b/apps/meteor/client/components/MarkdownText.tsx index 6a140f80a4909..acdc85f8a01ff 100644 --- a/apps/meteor/client/components/MarkdownText.tsx +++ b/apps/meteor/client/components/MarkdownText.tsx @@ -150,14 +150,14 @@ const MarkdownText = ({ } // Set appropriate title based on link type - if (isExternalLink) { - // For external links, set an empty title to prevent tooltips - // This reduces visual clutter and lets users see the URL in the browser's status bar instead - node.setAttribute('title', ''); - } else if (isMailto) { + if (isMailto) { // For mailto links, use the email address as the title for better user experience // Example: for href "mailto:user@example.com" the title would be "mailto:user@example.com" node.setAttribute('title', href); + } else if (isExternalLink) { + // For external links, set an empty title to prevent tooltips + // This reduces visual clutter and lets users see the URL in the browser's status bar instead + node.setAttribute('title', ''); } else { // 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" From 7fce7299ec2448c775cd7499ec9723d32ab75d97 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 14 May 2025 21:12:55 -0300 Subject: [PATCH 08/10] add schemes test --- .../client/components/MarkdownText.spec.tsx | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/apps/meteor/client/components/MarkdownText.spec.tsx b/apps/meteor/client/components/MarkdownText.spec.tsx index 16d87bb5f6d38..0320c7782b88b 100644 --- a/apps/meteor/client/components/MarkdownText.spec.tsx +++ b/apps/meteor/client/components/MarkdownText.spec.tsx @@ -1,10 +1,18 @@ import { mockAppRoot } from '@rocket.chat/mock-providers'; +import { isExternal } from '@rocket.chat/ui-client'; import { render } from '@testing-library/react'; import MarkdownText from './MarkdownText'; import '@testing-library/jest-dom'; +// Mock getBaseURI from @rocket.chat/ui-client to ensure consistent behavior in tests +// This will affect the isExternal function imported above. +jest.mock('@rocket.chat/ui-client', () => ({ + ...jest.requireActual('@rocket.chat/ui-client'), // Import and retain default behavior for other exports + getBaseURI: jest.fn(() => 'http://localhost/'), // Mock getBaseURI for consistent test behavior +})); + const normalizeHtml = (html: any) => { return html.replace(/\s+/g, ' ').trim(); }; @@ -116,3 +124,107 @@ it('should render html elements as expected using inline parser', async () => { expect(normalizedHtml).toContain('Italics within Bold text'); expect(normalizedHtml).toContain('Italics within Bold text with single underscore and asterik'); }); + +describe('MarkdownText scheme handling', () => { + // Initial data describing each scheme and its specific properties + const schemesData = [ + { schemeName: 'http-ext', link: 'http://example.com' }, + { schemeName: 'https-ext', link: 'https://example.com' }, + { schemeName: 'notes', link: 'notes://example.com/path' }, + { schemeName: 'ftp', link: 'ftp://example.com/file.txt' }, + { schemeName: 'ftps', link: 'ftps://example.com/file.txt' }, + { schemeName: 'tel', link: 'tel:+1234567890' }, + { schemeName: 'mailto', link: 'mailto:test@example.com', explicitTitle: 'mailto:test@example.com' }, + { schemeName: 'sms', link: 'sms:+1234567890?body=hello' }, + { schemeName: 'cid', link: 'cid:someimage@example.com' }, + { schemeName: 'internal-relative', link: '/channel/general', isRemoved: true }, + { schemeName: 'internal-absolute', link: 'http://localhost/another-channel' }, + { schemeName: 'invalid-scheme', link: 'invalid://example.com', isRemoved: true }, + { schemeName: 'javascript-uri', link: "javascript:alert('XSS')", isRemoved: true }, + ]; + + // Process schemesData to pre-calculate all expected attributes + const testCases = schemesData.map(({ schemeName, link, explicitTitle, isRemoved }) => { + const isActuallyExternal = isExternal(link); // Uses mocked getBaseURI + + let expectedTitleAttr: string | undefined = explicitTitle; + if (expectedTitleAttr === undefined) { + if (isRemoved) { + expectedTitleAttr = ''; // Removed/neutralized links get an empty title + } else { + expectedTitleAttr = isActuallyExternal ? '' : 'Go_to_href'; + } + } + + const shouldHaveExternalRelTarget = isRemoved || isActuallyExternal || link.startsWith('mailto:'); + + return { + schemeName, + link, + isRemoved: Boolean(isRemoved), + isActuallyExternal, + expectedHref: link, + expectedRel: shouldHaveExternalRelTarget ? 'nofollow noopener noreferrer' : undefined, + expectedTarget: shouldHaveExternalRelTarget ? '_blank' : undefined, + expectedTitleAttribute: expectedTitleAttr, + }; + }); + + testCases.forEach((tc) => { + it(`should ${tc.isRemoved ? 'handle' : 'correctly process'} ${tc.schemeName} links (isExternal returned: ${ + tc.isActuallyExternal + })`, () => { + const markdownContent = `[Test Link](${tc.link})`; + const { container } = render(, { + wrapper: mockAppRoot().build(), + }); + + const anchor = container.querySelector('a'); + expect(anchor).not.toBeNull(); + if (!anchor) return; // Type guard + + // Assert href + if (tc.isRemoved) { + const hrefAttr = anchor.getAttribute('href'); + if (tc.link.startsWith('javascript:')) { + expect(hrefAttr === null || hrefAttr === '#' || !hrefAttr.startsWith('javascript:')).toBe(true); + } else { + // For other 'invalid' or neutralized (like internal-relative) schemes. + // Expect href to be null (absent), '#', or the original link if DOMPurify didn't strip it. + expect(hrefAttr === null || hrefAttr === '#' || hrefAttr === tc.link).toBe(true); + } + } else { + // This 'else' block is for links that are not 'isRemoved' + expect(anchor.getAttribute('href')).toBe(tc.expectedHref); + } + + // Assert rel + if (tc.expectedRel) { + expect(anchor.getAttribute('rel')).toBe(tc.expectedRel); + } else { + expect(anchor.hasAttribute('rel')).toBe(false); + } + + // Assert target + if (tc.expectedTarget) { + expect(anchor.getAttribute('target')).toBe(tc.expectedTarget); + } else { + expect(anchor.hasAttribute('target')).toBe(false); + } + + // Assert title + if (tc.expectedTitleAttribute === 'Go_to_href') { + // For internal links where title is a translation key + expect(anchor.hasAttribute('title')).toBe(true); + expect(anchor.getAttribute('title')).toBe('Go_to_href'); + } else if (tc.expectedTitleAttribute !== undefined) { + expect(anchor.getAttribute('title')).toBe(tc.expectedTitleAttribute); + } else { + // This case should ideally not be hit if expectedTitleAttribute is always defined by the mapping logic + expect(anchor.hasAttribute('title')).toBe(false); + } + + expect(anchor.textContent).toBe('Test Link'); + }); + }); +}); From 4db2f811490c9bfba633e94844930810a5ee057a Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Thu, 15 May 2025 12:03:40 -0300 Subject: [PATCH 09/10] fix lint --- .../client/components/MarkdownText.spec.tsx | 102 +++++++++++------- 1 file changed, 66 insertions(+), 36 deletions(-) diff --git a/apps/meteor/client/components/MarkdownText.spec.tsx b/apps/meteor/client/components/MarkdownText.spec.tsx index 0320c7782b88b..0d241c71e3d8f 100644 --- a/apps/meteor/client/components/MarkdownText.spec.tsx +++ b/apps/meteor/client/components/MarkdownText.spec.tsx @@ -1,6 +1,6 @@ import { mockAppRoot } from '@rocket.chat/mock-providers'; import { isExternal } from '@rocket.chat/ui-client'; -import { render } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; import MarkdownText from './MarkdownText'; @@ -126,7 +126,6 @@ it('should render html elements as expected using inline parser', async () => { }); describe('MarkdownText scheme handling', () => { - // Initial data describing each scheme and its specific properties const schemesData = [ { schemeName: 'http-ext', link: 'http://example.com' }, { schemeName: 'https-ext', link: 'https://example.com' }, @@ -143,21 +142,33 @@ describe('MarkdownText scheme handling', () => { { schemeName: 'javascript-uri', link: "javascript:alert('XSS')", isRemoved: true }, ]; - // Process schemesData to pre-calculate all expected attributes const testCases = schemesData.map(({ schemeName, link, explicitTitle, isRemoved }) => { - const isActuallyExternal = isExternal(link); // Uses mocked getBaseURI + const isActuallyExternal = isExternal(link); let expectedTitleAttr: string | undefined = explicitTitle; if (expectedTitleAttr === undefined) { if (isRemoved) { - expectedTitleAttr = ''; // Removed/neutralized links get an empty title + expectedTitleAttr = ''; + } else if (isActuallyExternal) { + expectedTitleAttr = ''; } else { - expectedTitleAttr = isActuallyExternal ? '' : 'Go_to_href'; + expectedTitleAttr = 'Go_to_href'; } } const shouldHaveExternalRelTarget = isRemoved || isActuallyExternal || link.startsWith('mailto:'); + let queryStrategy: { method: 'getByRole'; name: string | RegExp } | { method: 'getByText'; text: string | RegExp }; + + if (expectedTitleAttr && expectedTitleAttr !== '') { + queryStrategy = { method: 'getByRole', name: expectedTitleAttr }; + } else if (link.startsWith('mailto:') && !expectedTitleAttr) { + queryStrategy = { method: 'getByRole', name: link }; + } else { + // For links with empty titles or no specific title attribute, target by text content. + queryStrategy = { method: 'getByText', text: 'Test Link' }; + } + return { schemeName, link, @@ -167,64 +178,83 @@ describe('MarkdownText scheme handling', () => { expectedRel: shouldHaveExternalRelTarget ? 'nofollow noopener noreferrer' : undefined, expectedTarget: shouldHaveExternalRelTarget ? '_blank' : undefined, expectedTitleAttribute: expectedTitleAttr, + queryStrategy, }; }); testCases.forEach((tc) => { - it(`should ${tc.isRemoved ? 'handle' : 'correctly process'} ${tc.schemeName} links (isExternal returned: ${ - tc.isActuallyExternal - })`, () => { + it(`should ${tc.isRemoved ? 'handle' : 'correctly process'} ${tc.schemeName} links (isExternal returned: ${tc.isActuallyExternal})`, () => { const markdownContent = `[Test Link](${tc.link})`; - const { container } = render(, { + render(, { wrapper: mockAppRoot().build(), }); - const anchor = container.querySelector('a'); - expect(anchor).not.toBeNull(); - if (!anchor) return; // Type guard + let anchorElement: HTMLElement; + if (tc.queryStrategy.method === 'getByRole') { + anchorElement = screen.getByRole('link', { name: tc.queryStrategy.name }); + } else { + // tc.queryStrategy.method === 'getByText' + const elementWithText = screen.getByText(tc.queryStrategy.text); + if (elementWithText.tagName === 'A') { + anchorElement = elementWithText; + } else { + const parentAnchor = elementWithText.closest('a'); + if (!parentAnchor) { + // This case should ideally not happen if markdown `[Text](link)` is parsed correctly to an anchor. + // If it does, it indicates a deeper issue or a test case where text isn't in an anchor. + throw new Error(`Query by text found "${tc.queryStrategy.text}", but it is not within an anchor tag.`); + } + anchorElement = parentAnchor; + } + } + + expect(anchorElement).toBeInTheDocument(); + expect(anchorElement.tagName).toBe('A'); // Assert href if (tc.isRemoved) { - const hrefAttr = anchor.getAttribute('href'); + // eslint-disable-next-line testing-library/no-node-access -- Need to check href value against multiple possibilities for sanitized links + const currentHref = anchorElement.getAttribute('href'); if (tc.link.startsWith('javascript:')) { - expect(hrefAttr === null || hrefAttr === '#' || !hrefAttr.startsWith('javascript:')).toBe(true); + // For JS links, ensure href is neutralized (null, '#', or non-JS) + expect( + currentHref === null || // Stripped completely + currentHref === '#' || // Commonly neutralized to # + !currentHref.startsWith('javascript:'), // Original but non-functional due to other attributes or browser measures + ).toBe(true); + } else if (tc.schemeName === 'internal-relative') { + // Internal relative links are expected to have their href attribute removed by DOMPurify + expect(anchorElement).not.toHaveAttribute('href'); } else { - // For other 'invalid' or neutralized (like internal-relative) schemes. - // Expect href to be null (absent), '#', or the original link if DOMPurify didn't strip it. - expect(hrefAttr === null || hrefAttr === '#' || hrefAttr === tc.link).toBe(true); + // For other removed links (e.g., invalid-scheme), href might be original, '#', or null. + expect( + currentHref === tc.link || // Left as is + currentHref === '#' || // Neutralized to # + currentHref === null, // Stripped + ).toBe(true); } } else { - // This 'else' block is for links that are not 'isRemoved' - expect(anchor.getAttribute('href')).toBe(tc.expectedHref); + expect(anchorElement).toHaveAttribute('href', tc.expectedHref); } // Assert rel if (tc.expectedRel) { - expect(anchor.getAttribute('rel')).toBe(tc.expectedRel); + expect(anchorElement).toHaveAttribute('rel', tc.expectedRel); } else { - expect(anchor.hasAttribute('rel')).toBe(false); + expect(anchorElement).not.toHaveAttribute('rel'); } // Assert target if (tc.expectedTarget) { - expect(anchor.getAttribute('target')).toBe(tc.expectedTarget); + expect(anchorElement).toHaveAttribute('target', tc.expectedTarget); } else { - expect(anchor.hasAttribute('target')).toBe(false); + expect(anchorElement).not.toHaveAttribute('target'); } - // Assert title - if (tc.expectedTitleAttribute === 'Go_to_href') { - // For internal links where title is a translation key - expect(anchor.hasAttribute('title')).toBe(true); - expect(anchor.getAttribute('title')).toBe('Go_to_href'); - } else if (tc.expectedTitleAttribute !== undefined) { - expect(anchor.getAttribute('title')).toBe(tc.expectedTitleAttribute); - } else { - // This case should ideally not be hit if expectedTitleAttribute is always defined by the mapping logic - expect(anchor.hasAttribute('title')).toBe(false); - } + // Assert title - tc.expectedTitleAttribute is always a string ('' or a specific title) + expect(anchorElement).toHaveAttribute('title', tc.expectedTitleAttribute); - expect(anchor.textContent).toBe('Test Link'); + expect(anchorElement).toHaveTextContent('Test Link'); }); }); }); From 6658c2eef258e6512226e273c7b56bcdaf77b655 Mon Sep 17 00:00:00 2001 From: Tasso Date: Sun, 18 May 2025 00:46:03 -0300 Subject: [PATCH 10/10] Explicit test cases --- .../client/components/MarkdownText.spec.tsx | 262 +++++++++--------- 1 file changed, 136 insertions(+), 126 deletions(-) diff --git a/apps/meteor/client/components/MarkdownText.spec.tsx b/apps/meteor/client/components/MarkdownText.spec.tsx index 0d241c71e3d8f..a2aaaeaea1ede 100644 --- a/apps/meteor/client/components/MarkdownText.spec.tsx +++ b/apps/meteor/client/components/MarkdownText.spec.tsx @@ -1,5 +1,4 @@ import { mockAppRoot } from '@rocket.chat/mock-providers'; -import { isExternal } from '@rocket.chat/ui-client'; import { render, screen } from '@testing-library/react'; import MarkdownText from './MarkdownText'; @@ -125,136 +124,147 @@ it('should render html elements as expected using inline parser', async () => { expect(normalizedHtml).toContain('Italics within Bold text with single underscore and asterik'); }); -describe('MarkdownText scheme handling', () => { - const schemesData = [ - { schemeName: 'http-ext', link: 'http://example.com' }, - { schemeName: 'https-ext', link: 'https://example.com' }, - { schemeName: 'notes', link: 'notes://example.com/path' }, - { schemeName: 'ftp', link: 'ftp://example.com/file.txt' }, - { schemeName: 'ftps', link: 'ftps://example.com/file.txt' }, - { schemeName: 'tel', link: 'tel:+1234567890' }, - { schemeName: 'mailto', link: 'mailto:test@example.com', explicitTitle: 'mailto:test@example.com' }, - { schemeName: 'sms', link: 'sms:+1234567890?body=hello' }, - { schemeName: 'cid', link: 'cid:someimage@example.com' }, - { schemeName: 'internal-relative', link: '/channel/general', isRemoved: true }, - { schemeName: 'internal-absolute', link: 'http://localhost/another-channel' }, - { schemeName: 'invalid-scheme', link: 'invalid://example.com', isRemoved: true }, - { schemeName: 'javascript-uri', link: "javascript:alert('XSS')", isRemoved: true }, - ]; - - const testCases = schemesData.map(({ schemeName, link, explicitTitle, isRemoved }) => { - const isActuallyExternal = isExternal(link); - - let expectedTitleAttr: string | undefined = explicitTitle; - if (expectedTitleAttr === undefined) { - if (isRemoved) { - expectedTitleAttr = ''; - } else if (isActuallyExternal) { - expectedTitleAttr = ''; - } else { - expectedTitleAttr = 'Go_to_href'; - } - } - - const shouldHaveExternalRelTarget = isRemoved || isActuallyExternal || link.startsWith('mailto:'); - - let queryStrategy: { method: 'getByRole'; name: string | RegExp } | { method: 'getByText'; text: string | RegExp }; - - if (expectedTitleAttr && expectedTitleAttr !== '') { - queryStrategy = { method: 'getByRole', name: expectedTitleAttr }; - } else if (link.startsWith('mailto:') && !expectedTitleAttr) { - queryStrategy = { method: 'getByRole', name: link }; - } else { - // For links with empty titles or no specific title attribute, target by text content. - queryStrategy = { method: 'getByText', text: 'Test Link' }; - } - - return { - schemeName, - link, - isRemoved: Boolean(isRemoved), - isActuallyExternal, - expectedHref: link, - expectedRel: shouldHaveExternalRelTarget ? 'nofollow noopener noreferrer' : undefined, - expectedTarget: shouldHaveExternalRelTarget ? '_blank' : undefined, - expectedTitleAttribute: expectedTitleAttr, - queryStrategy, - }; - }); - - testCases.forEach((tc) => { - it(`should ${tc.isRemoved ? 'handle' : 'correctly process'} ${tc.schemeName} links (isExternal returned: ${tc.isActuallyExternal})`, () => { - const markdownContent = `[Test Link](${tc.link})`; - render(, { - wrapper: mockAppRoot().build(), - }); - - let anchorElement: HTMLElement; - if (tc.queryStrategy.method === 'getByRole') { - anchorElement = screen.getByRole('link', { name: tc.queryStrategy.name }); - } else { - // tc.queryStrategy.method === 'getByText' - const elementWithText = screen.getByText(tc.queryStrategy.text); - if (elementWithText.tagName === 'A') { - anchorElement = elementWithText; - } else { - const parentAnchor = elementWithText.closest('a'); - if (!parentAnchor) { - // This case should ideally not happen if markdown `[Text](link)` is parsed correctly to an anchor. - // If it does, it indicates a deeper issue or a test case where text isn't in an anchor. - throw new Error(`Query by text found "${tc.queryStrategy.text}", but it is not within an anchor tag.`); - } - anchorElement = parentAnchor; - } - } +describe('links handling', () => { + it.each([ + { + caseName: 'transform external http', + link: 'http://example.com', + query: () => screen.getByRole('link', { name: 'Test Link' }), + expectedHref: 'http://example.com', + expectedRel: 'nofollow noopener noreferrer', + expectedTarget: '_blank', + expectedTitleAttribute: '', + }, + { + caseName: 'transform external https', + link: 'https://example.com', + query: () => screen.getByRole('link', { name: 'Test Link' }), + expectedHref: 'https://example.com', + expectedRel: 'nofollow noopener noreferrer', + expectedTarget: '_blank', + expectedTitleAttribute: '', + }, + { + caseName: 'transform notes', + link: 'notes://example.com/path', + query: () => screen.getByRole('link', { name: 'Test Link' }), + expectedHref: 'notes://example.com/path', + expectedRel: 'nofollow noopener noreferrer', + expectedTarget: '_blank', + expectedTitleAttribute: '', + }, + { + caseName: 'transform ftp', + link: 'ftp://example.com/file.txt', + query: () => screen.getByRole('link', { name: 'Test Link' }), + expectedHref: 'ftp://example.com/file.txt', + expectedRel: 'nofollow noopener noreferrer', + expectedTarget: '_blank', + expectedTitleAttribute: '', + }, + { + caseName: 'transform ftps', + link: 'ftps://example.com/file.txt', + query: () => screen.getByRole('link', { name: 'Test Link' }), + expectedHref: 'ftps://example.com/file.txt', + expectedRel: 'nofollow noopener noreferrer', + expectedTarget: '_blank', + expectedTitleAttribute: '', + }, + { + caseName: 'transform tel', + link: 'tel:+1234567890', + query: () => screen.getByRole('link', { name: 'Test Link' }), + expectedHref: 'tel:+1234567890', + expectedRel: 'nofollow noopener noreferrer', + expectedTarget: '_blank', + expectedTitleAttribute: '', + }, + { + caseName: 'transform mailto', + link: 'mailto:test@example.com', + query: () => screen.getByRole('link', { name: 'mailto:test@example.com' }), + expectedHref: 'mailto:test@example.com', + expectedRel: 'nofollow noopener noreferrer', + expectedTarget: '_blank', + expectedTitleAttribute: 'mailto:test@example.com', + }, + { + caseName: 'transform sms', + link: 'sms:+1234567890?body=hello', + query: () => screen.getByRole('link', { name: 'Test Link' }), + expectedHref: 'sms:+1234567890?body=hello', + expectedRel: 'nofollow noopener noreferrer', + expectedTarget: '_blank', + expectedTitleAttribute: '', + }, + { + caseName: 'transform cid', + link: 'cid:someimage@example.com', + query: () => screen.getByRole('link', { name: 'Test Link' }), + expectedHref: 'cid:someimage@example.com', + expectedRel: 'nofollow noopener noreferrer', + expectedTarget: '_blank', + expectedTitleAttribute: '', + }, + { + caseName: 'filter relative', + link: '/channel/general', + query: () => screen.getByText('Test Link'), + expectedHref: undefined, + expectedRel: 'nofollow noopener noreferrer', + expectedTarget: '_blank', + expectedTitleAttribute: '', + }, + { + caseName: 'transform absolute', + link: 'http://localhost/another-channel', + query: () => screen.getByRole('link', { name: 'Go to: another-channel' }), + expectedHref: 'http://localhost/another-channel', + expectedRel: undefined, + expectedTarget: undefined, + expectedTitleAttribute: 'Go to: another-channel', + }, + { + caseName: 'filter unknown scheme', + link: 'invalid://example.com', + query: () => screen.getByText('Test Link'), + expectedHref: undefined, + expectedRel: 'nofollow noopener noreferrer', + expectedTarget: '_blank', + expectedTitleAttribute: '', + }, + { + caseName: 'filter javascript', + link: "javascript:alert('XSS')", + query: () => screen.getByText('Test Link'), + expectedHref: undefined, + expectedRel: 'nofollow noopener noreferrer', + expectedTarget: '_blank', + expectedTitleAttribute: '', + }, + ] as const)('should $caseName links', ({ link, query, expectedHref, expectedRel, expectedTarget, expectedTitleAttribute }) => { + const markdownContent = `[Test Link](${link})`; + render(, { + wrapper: mockAppRoot().withTranslations('en', 'core', { Go_to_href: 'Go to: {{href}}' }).build(), + }); - expect(anchorElement).toBeInTheDocument(); - expect(anchorElement.tagName).toBe('A'); + const anchorElement = query(); - // Assert href - if (tc.isRemoved) { - // eslint-disable-next-line testing-library/no-node-access -- Need to check href value against multiple possibilities for sanitized links - const currentHref = anchorElement.getAttribute('href'); - if (tc.link.startsWith('javascript:')) { - // For JS links, ensure href is neutralized (null, '#', or non-JS) - expect( - currentHref === null || // Stripped completely - currentHref === '#' || // Commonly neutralized to # - !currentHref.startsWith('javascript:'), // Original but non-functional due to other attributes or browser measures - ).toBe(true); - } else if (tc.schemeName === 'internal-relative') { - // Internal relative links are expected to have their href attribute removed by DOMPurify - expect(anchorElement).not.toHaveAttribute('href'); - } else { - // For other removed links (e.g., invalid-scheme), href might be original, '#', or null. - expect( - currentHref === tc.link || // Left as is - currentHref === '#' || // Neutralized to # - currentHref === null, // Stripped - ).toBe(true); - } - } else { - expect(anchorElement).toHaveAttribute('href', tc.expectedHref); - } + expect(anchorElement).toBeInTheDocument(); + expect(anchorElement.tagName).toBe('A'); + expect(anchorElement).toHaveTextContent('Test Link'); - // Assert rel - if (tc.expectedRel) { - expect(anchorElement).toHaveAttribute('rel', tc.expectedRel); - } else { - expect(anchorElement).not.toHaveAttribute('rel'); - } + if (expectedHref !== undefined) expect(anchorElement).toHaveAttribute('href', expectedHref); + else expect(anchorElement).not.toHaveAttribute('href'); - // Assert target - if (tc.expectedTarget) { - expect(anchorElement).toHaveAttribute('target', tc.expectedTarget); - } else { - expect(anchorElement).not.toHaveAttribute('target'); - } + if (expectedRel !== undefined) expect(anchorElement).toHaveAttribute('rel', expectedRel); + else expect(anchorElement).not.toHaveAttribute('rel'); - // Assert title - tc.expectedTitleAttribute is always a string ('' or a specific title) - expect(anchorElement).toHaveAttribute('title', tc.expectedTitleAttribute); + if (expectedTarget !== undefined) expect(anchorElement).toHaveAttribute('target', expectedTarget); + else expect(anchorElement).not.toHaveAttribute('target'); - expect(anchorElement).toHaveTextContent('Test Link'); - }); + if (expectedTitleAttribute !== undefined) expect(anchorElement).toHaveAttribute('title', expectedTitleAttribute); + else expect(anchorElement).not.toHaveAttribute('title'); }); });