From da5fb2e4c4644bafeb5269971a878f86d482b25d Mon Sep 17 00:00:00 2001 From: rebelchris Date: Mon, 21 Sep 2026 14:32:02 +0000 Subject: [PATCH 1/3] fix(markdown): prevent image links from trapping users --- .../shared/src/components/Markdown.spec.tsx | 189 ++++++++++++++++++ packages/shared/src/components/Markdown.tsx | 106 +++++++++- packages/shared/src/lib/image.spec.ts | 37 +++- packages/shared/src/lib/image.ts | 32 +++ 4 files changed, 353 insertions(+), 11 deletions(-) create mode 100644 packages/shared/src/components/Markdown.spec.tsx diff --git a/packages/shared/src/components/Markdown.spec.tsx b/packages/shared/src/components/Markdown.spec.tsx new file mode 100644 index 00000000000..a97084974a9 --- /dev/null +++ b/packages/shared/src/components/Markdown.spec.tsx @@ -0,0 +1,189 @@ +import React from 'react'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { fireEvent, render, screen } from '@testing-library/react'; +import Markdown from './Markdown'; +import { LazyModal } from './modals/common/types'; +import { useRequestProtocol } from '../hooks/useRequestProtocol'; + +const mockOpenModal = jest.fn(); + +jest.mock('../hooks/useLazyModal', () => ({ + useLazyModal: () => ({ openModal: mockOpenModal }), +})); + +jest.mock('../hooks/useDomPurify', () => ({ + useDomPurify: () => ({ sanitize: (content: string) => content }), +})); + +jest.mock('../hooks/useRequestProtocol', () => ({ + useRequestProtocol: jest.fn(), +})); + +const mockUseRequestProtocol = useRequestProtocol as jest.MockedFunction< + typeof useRequestProtocol +>; + +const renderMarkdown = (content: string) => { + const client = new QueryClient(); + + return render( + + + , + ); +}; + +describe('Markdown image interactions', () => { + const imageUrl = 'https://media.daily.dev/image/upload/f_auto/v1/posts/abc'; + + beforeEach(() => { + jest.clearAllMocks(); + mockUseRequestProtocol.mockReturnValue({ + requestMethod: jest.fn(), + fetchMethod: jest.fn(), + isCompanion: false, + } as unknown as ReturnType); + }); + + it('opens a linked image in the lightbox and prevents navigation', () => { + renderMarkdown( + `Screenshot`, + ); + + const image = screen.getByRole('button', { name: 'Open image' }); + const event = new MouseEvent('click', { + bubbles: true, + cancelable: true, + }); + + image.dispatchEvent(event); + + expect(event.defaultPrevented).toBe(true); + expect(mockOpenModal).toHaveBeenCalledTimes(1); + expect(mockOpenModal).toHaveBeenCalledWith( + expect.objectContaining({ + type: LazyModal.ImageView, + props: expect.objectContaining({ + src: imageUrl, + alt: 'Screenshot', + originRect: expect.objectContaining({ + top: expect.any(Number), + left: expect.any(Number), + width: expect.any(Number), + height: expect.any(Number), + }), + }), + }), + ); + }); + + it('opens a linked image from the keyboard and prevents navigation', () => { + renderMarkdown( + `Screenshot`, + ); + + const image = screen.getByRole('button', { name: 'Open image' }); + + expect(fireEvent.keyDown(image, { key: 'Enter' })).toBe(false); + expect(mockOpenModal).toHaveBeenCalledTimes(1); + expect(mockOpenModal).toHaveBeenCalledWith( + expect.objectContaining({ + type: LazyModal.ImageView, + props: expect.objectContaining({ + src: imageUrl, + alt: 'Screenshot', + }), + }), + ); + }); + + it('allows an image wrapped by a non-image link to follow that link once', () => { + const linkUrl = 'https://github.com/dailydotdev/apps'; + renderMarkdown( + `Repository badge`, + ); + + const image = screen.getByRole('button', { name: 'Open image' }); + const anchor = image.closest('a'); + const onAnchorClick = jest.fn(); + + anchor?.addEventListener('click', onAnchorClick); + fireEvent.click(image); + + expect(onAnchorClick).toHaveBeenCalledTimes(1); + expect(mockOpenModal).not.toHaveBeenCalled(); + }); + + it('allows an image wrapped by a non-image link to follow that link from the keyboard', () => { + const linkUrl = 'https://github.com/dailydotdev/apps'; + renderMarkdown( + `Repository badge`, + ); + + const image = screen.getByRole('button', { name: 'Open image' }); + const anchor = image.closest('a'); + const onAnchorClick = jest.fn(); + + anchor?.addEventListener('click', onAnchorClick); + expect(fireEvent.keyDown(image, { key: 'Enter' })).toBe(false); + + expect(onAnchorClick).toHaveBeenCalledTimes(1); + expect(mockOpenModal).not.toHaveBeenCalled(); + }); + + it('opens a bare image in the lightbox', () => { + renderMarkdown(`Screenshot`); + + fireEvent.click(screen.getByRole('button', { name: 'Open image' })); + + expect(mockOpenModal).toHaveBeenCalledTimes(1); + expect(mockOpenModal).toHaveBeenCalledWith( + expect.objectContaining({ + type: LazyModal.ImageView, + props: expect.objectContaining({ + src: imageUrl, + alt: 'Screenshot', + }), + }), + ); + }); + + it('opens a text link to an image in the lightbox', () => { + renderMarkdown( + `screenshot`, + ); + + fireEvent.click(screen.getByRole('link', { name: 'screenshot' })); + + expect(mockOpenModal).toHaveBeenCalledTimes(1); + expect(mockOpenModal).toHaveBeenCalledWith( + expect.objectContaining({ + type: LazyModal.ImageView, + props: expect.objectContaining({ + src: 'https://example.com/screenshot.png?raw=1', + alt: 'screenshot', + }), + }), + ); + }); + + it('opens image links in a new tab for the companion', () => { + mockUseRequestProtocol.mockReturnValue({ + requestMethod: jest.fn(), + fetchMethod: jest.fn(), + isCompanion: true, + } as unknown as ReturnType); + + renderMarkdown(`Screenshot`); + + fireEvent.click(screen.getByRole('button', { name: 'Open image' })); + + expect(window.open).toHaveBeenCalledTimes(1); + expect(window.open).toHaveBeenCalledWith( + imageUrl, + '_blank', + 'noopener,noreferrer', + ); + expect(mockOpenModal).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/shared/src/components/Markdown.tsx b/packages/shared/src/components/Markdown.tsx index e1c82bb571c..57dd49f9421 100644 --- a/packages/shared/src/components/Markdown.tsx +++ b/packages/shared/src/components/Markdown.tsx @@ -18,6 +18,7 @@ import { useLazyModal } from '../hooks/useLazyModal'; import { LazyModal } from './modals/common/types'; import { getImageOriginRect } from './modals/ImageModal'; import { useRequestProtocol } from '../hooks/useRequestProtocol'; +import { isImageUrl } from '../lib/image'; function isImageElement( element: Element | EventTarget, @@ -25,6 +26,54 @@ function isImageElement( return element instanceof HTMLImageElement; } +function getTargetElement(target: EventTarget): Element | null { + if (target instanceof Element) { + return target; + } + + if (target instanceof Node) { + return target.parentElement; + } + + return null; +} + +function getWrappingAnchor( + element: Element, + container: HTMLElement | null, +): HTMLAnchorElement | null { + let currentElement: Element | null = element; + + while (currentElement && currentElement !== container) { + if (currentElement instanceof HTMLAnchorElement) { + return currentElement; + } + + currentElement = currentElement.parentElement; + } + + return null; +} + +function isSameUrl(url: string, otherUrl: string): boolean { + try { + return new URL(url).href === new URL(otherUrl).href; + } catch { + return url === otherUrl; + } +} + +function shouldOpenAnchorImage( + anchor: HTMLAnchorElement | null, + imageSrc: string, +): boolean { + if (!anchor?.href) { + return true; + } + + return isImageUrl(anchor.href) || isSameUrl(anchor.href, imageSrc); +} + const UserEntityCard = dynamic(() => import('./cards/entity/UserEntityCard'), { ssr: false, }); @@ -120,19 +169,19 @@ export default function Markdown({ ); const openImage = useCallback( - (element: HTMLImageElement) => { + (src: string, alt: string | undefined, originElement: Element) => { // The lazy-modal renderer isn't mounted in the extension companion, so // fall back to opening the image in a new tab there. if (isCompanion) { - window.open(element.src, '_blank', 'noopener,noreferrer'); + window.open(src, '_blank', 'noopener,noreferrer'); return; } openModal({ type: LazyModal.ImageView, props: { - src: element.src, - alt: element.alt || undefined, - originRect: getImageOriginRect(element), + src, + alt, + originRect: getImageOriginRect(originElement), }, }); }, @@ -141,11 +190,34 @@ export default function Markdown({ const onImageClick = useCallback( (e: MouseEvent) => { - const element = e.target; + const element = getTargetElement(e.target); + + if (!element) { + return; + } + + const anchor = getWrappingAnchor(element, containerRef.current); if (isImageElement(element) && element.src) { e.stopPropagation(); - openImage(element); + + if (!shouldOpenAnchorImage(anchor, element.src)) { + return; + } + + e.preventDefault(); + openImage( + anchor?.href || element.src, + element.alt || undefined, + element, + ); + return; + } + + if (anchor?.href && isImageUrl(anchor.href)) { + e.preventDefault(); + e.stopPropagation(); + openImage(anchor.href, anchor.textContent?.trim() || undefined, anchor); } }, [openImage], @@ -153,16 +225,30 @@ export default function Markdown({ const onImageKeyDown = useCallback( (e: KeyboardEvent) => { - const element = e.target; + const element = getTargetElement(e.target); if ( + element && isImageElement(element) && element.src && (e.key === 'Enter' || e.key === ' ') ) { - e.preventDefault(); + const anchor = getWrappingAnchor(element, containerRef.current); + e.stopPropagation(); - openImage(element); + + if (!shouldOpenAnchorImage(anchor, element.src)) { + e.preventDefault(); + anchor?.click(); + return; + } + + e.preventDefault(); + openImage( + anchor?.href || element.src, + element.alt || undefined, + element, + ); } }, [openImage], diff --git a/packages/shared/src/lib/image.spec.ts b/packages/shared/src/lib/image.spec.ts index 7e0f91dbe69..61e67a18d66 100644 --- a/packages/shared/src/lib/image.spec.ts +++ b/packages/shared/src/lib/image.spec.ts @@ -1,4 +1,4 @@ -import { isPlaceholderImage } from './image'; +import { isImageUrl, isPlaceholderImage } from './image'; describe('isPlaceholderImage', () => { it('returns true for API placeholder images', () => { @@ -42,3 +42,38 @@ describe('isPlaceholderImage', () => { expect(isPlaceholderImage('not-a-url')).toBe(false); }); }); + +describe('isImageUrl', () => { + it('matches image file extensions with query strings and fragments', () => { + expect(isImageUrl('https://example.com/screenshot.PNG?width=800#top')).toBe( + true, + ); + expect(isImageUrl('https://example.com/photo.avif')).toBe(true); + expect(isImageUrl('https://example.com/icon.svg')).toBe(true); + }); + + it('matches daily media and Cloudinary image URLs without extensions', () => { + expect( + isImageUrl('https://media.daily.dev/image/upload/f_auto/v1/posts/abc'), + ).toBe(true); + expect( + isImageUrl('https://res.cloudinary.com/daily-now/image/upload/abc'), + ).toBe(true); + expect( + isImageUrl('https://daily-now-res.cloudinary.com/image/upload/abc'), + ).toBe(true); + }); + + it('matches protocol-relative and relative image URLs', () => { + expect(isImageUrl('//cdn.example.com/image.webp')).toBe(true); + expect(isImageUrl('/uploads/image.jpg?raw=1')).toBe(true); + expect(isImageUrl('../assets/image.gif')).toBe(true); + }); + + it('rejects non-image URLs', () => { + expect(isImageUrl('https://example.com/articles/123')).toBe(false); + expect( + isImageUrl('https://res.cloudinary.com/daily-now/raw/upload/abc'), + ).toBe(false); + }); +}); diff --git a/packages/shared/src/lib/image.ts b/packages/shared/src/lib/image.ts index 46e748da62c..9659a24d15c 100644 --- a/packages/shared/src/lib/image.ts +++ b/packages/shared/src/lib/image.ts @@ -1,5 +1,37 @@ const DAILY_MEDIA_HOST = 'media.daily.dev'; const PLACEHOLDER_IMAGE_PATTERN = /placeholder/i; +const IMAGE_FILE_EXTENSION_PATTERN = + /\.(?:apng|avif|bmp|cur|gif|heic|heif|ico|jfif|jpe?g|pjp|pjpeg|png|svg|tiff?|webp)$/i; +const CLOUDINARY_IMAGE_HOST_PATTERN = /(^|\.)cloudinary\.com$/; + +export const isImageUrl = ( + url: string, + baseUrl = globalThis.location?.href ?? 'https://daily.dev', +): boolean => { + if (!url) { + return false; + } + + try { + const parsedUrl = new URL(url, baseUrl); + const host = parsedUrl.hostname.toLowerCase(); + + if (host === DAILY_MEDIA_HOST) { + return true; + } + + if ( + CLOUDINARY_IMAGE_HOST_PATTERN.test(host) && + parsedUrl.pathname.includes('/image/') + ) { + return true; + } + + return IMAGE_FILE_EXTENSION_PATTERN.test(parsedUrl.pathname); + } catch { + return false; + } +}; export const cloudinaryPostImageCoverPlaceholder = 'https://media.daily.dev/image/upload/s--P4t4XyoV--/f_auto/v1722860399/public/Placeholder%2001'; From 20e504b5cfc8880d23530be0d716e038cba527ee Mon Sep 17 00:00:00 2001 From: Chris Bongers Date: Tue, 22 Sep 2026 08:39:29 +0200 Subject: [PATCH 2/3] test: wait for streak recovery modal to render Network mocks can finish before React commits the recovery modal. Await the modal heading in positive assertions so the shared test suite does not fail depending on render timing. --- .../modals/streaks/StreakRecoverModal.spec.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/shared/src/components/modals/streaks/StreakRecoverModal.spec.tsx b/packages/shared/src/components/modals/streaks/StreakRecoverModal.spec.tsx index b657fe4e9bc..5b18f8c6eba 100644 --- a/packages/shared/src/components/modals/streaks/StreakRecoverModal.spec.tsx +++ b/packages/shared/src/components/modals/streaks/StreakRecoverModal.spec.tsx @@ -202,7 +202,7 @@ it('should render and fetch initial data if logged user can recover streak', asy expect(haveFetched).toBeTruthy(); // and rendered - const popup = screen.queryByTestId('streak-recover-modal-heading'); + const popup = await screen.findByTestId('streak-recover-modal-heading'); expect(popup).toBeInTheDocument(); }); @@ -254,7 +254,7 @@ it('Should have no cost for first time recovery', async () => { await waitForNock(); // rendered - const popupHeader = screen.queryByTestId('streak-recover-modal-heading'); + const popupHeader = await screen.findByTestId('streak-recover-modal-heading'); expect(popupHeader).toBeInTheDocument(); // expect cost to be 0 @@ -282,7 +282,7 @@ it('Should have cost of 100 Cores for 2nd+ time recovery', async () => { await waitForNock(); // rendered - const popupHeader = screen.queryByTestId('streak-recover-modal-heading'); + const popupHeader = await screen.findByTestId('streak-recover-modal-heading'); expect(popupHeader).toBeInTheDocument(); // expect cost to be 100 @@ -310,7 +310,7 @@ it('Should show buy Cores message if user does not have enough Cores', async () await waitForNock(); // rendered - const popupHeader = screen.queryByTestId('streak-recover-modal-heading'); + const popupHeader = await screen.findByTestId('streak-recover-modal-heading'); expect(popupHeader).toBeInTheDocument(); // expect not enough Cores message @@ -344,7 +344,7 @@ it('Should show success message on recover', async () => { await waitForNock(); // rendered - const popupHeader = screen.queryByTestId('streak-recover-modal-heading'); + const popupHeader = await screen.findByTestId('streak-recover-modal-heading'); expect(popupHeader).toBeInTheDocument(); // button is there From b0f8fa3066ca03018b7bb4eb67edd5b43fbb709e Mon Sep 17 00:00:00 2001 From: Chris Bongers Date: Tue, 22 Sep 2026 08:46:42 +0200 Subject: [PATCH 3/3] fix(markdown): preserve video and modified link navigation Require the image path for extensionless daily media URLs so video links remain navigable. Leave modified and non-primary clicks to the browser. Cover video classification and link navigation in regression tests, using detached containers to avoid the test harness's global link cancellation. --- .../shared/src/components/Markdown.spec.tsx | 46 ++++++++++++++++++- packages/shared/src/components/Markdown.tsx | 4 ++ packages/shared/src/lib/image.spec.ts | 8 ++++ packages/shared/src/lib/image.ts | 6 +-- 4 files changed, 57 insertions(+), 7 deletions(-) diff --git a/packages/shared/src/components/Markdown.spec.tsx b/packages/shared/src/components/Markdown.spec.tsx index a97084974a9..94802bf7cc3 100644 --- a/packages/shared/src/components/Markdown.spec.tsx +++ b/packages/shared/src/components/Markdown.spec.tsx @@ -1,6 +1,6 @@ import React from 'react'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; -import { fireEvent, render, screen } from '@testing-library/react'; +import { fireEvent, render, screen, within } from '@testing-library/react'; import Markdown from './Markdown'; import { LazyModal } from './modals/common/types'; import { useRequestProtocol } from '../hooks/useRequestProtocol'; @@ -23,13 +23,14 @@ const mockUseRequestProtocol = useRequestProtocol as jest.MockedFunction< typeof useRequestProtocol >; -const renderMarkdown = (content: string) => { +const renderMarkdown = (content: string, container?: HTMLElement) => { const client = new QueryClient(); return render( , + { container }, ); }; @@ -167,6 +168,47 @@ describe('Markdown image interactions', () => { ); }); + it('allows text links to daily media videos to navigate', () => { + const { container } = renderMarkdown( + 'Watch video', + document.createElement('div'), + ); + const { getByRole } = within(container); + + expect(fireEvent.click(getByRole('link', { name: 'Watch video' }))).toBe( + true, + ); + expect(mockOpenModal).not.toHaveBeenCalled(); + }); + + describe.each(['image', 'text'])('%s links', (contentType) => { + it.each([ + { metaKey: true }, + { ctrlKey: true }, + { shiftKey: true }, + { altKey: true }, + { button: 1 }, + ])('preserves browser navigation for clicks with %j', (options) => { + const content = + contentType === 'image' + ? `Screenshot` + : 'Screenshot'; + const { container } = renderMarkdown( + `${content}`, + document.createElement('div'), + ); + const { getByRole } = within(container); + + const target = + contentType === 'image' + ? getByRole('button', { name: 'Open image' }) + : getByRole('link', { name: 'Screenshot' }); + + expect(fireEvent.click(target, options)).toBe(true); + expect(mockOpenModal).not.toHaveBeenCalled(); + }); + }); + it('opens image links in a new tab for the companion', () => { mockUseRequestProtocol.mockReturnValue({ requestMethod: jest.fn(), diff --git a/packages/shared/src/components/Markdown.tsx b/packages/shared/src/components/Markdown.tsx index 57dd49f9421..d1d3b2c115b 100644 --- a/packages/shared/src/components/Markdown.tsx +++ b/packages/shared/src/components/Markdown.tsx @@ -190,6 +190,10 @@ export default function Markdown({ const onImageClick = useCallback( (e: MouseEvent) => { + if (e.metaKey || e.ctrlKey || e.shiftKey || e.altKey || e.button !== 0) { + return; + } + const element = getTargetElement(e.target); if (!element) { diff --git a/packages/shared/src/lib/image.spec.ts b/packages/shared/src/lib/image.spec.ts index 61e67a18d66..ce940aaaf4d 100644 --- a/packages/shared/src/lib/image.spec.ts +++ b/packages/shared/src/lib/image.spec.ts @@ -76,4 +76,12 @@ describe('isImageUrl', () => { isImageUrl('https://res.cloudinary.com/daily-now/raw/upload/abc'), ).toBe(false); }); + + it.each([ + 'https://media.daily.dev/video/upload/v1/posts/clip.mp4', + 'https://media.daily.dev/video/upload/v1/posts/clip', + 'https://media.daily.dev/raw/upload/v1/posts/document', + ])('rejects non-image daily media URLs: %s', (url) => { + expect(isImageUrl(url)).toBe(false); + }); }); diff --git a/packages/shared/src/lib/image.ts b/packages/shared/src/lib/image.ts index 9659a24d15c..a326b99638f 100644 --- a/packages/shared/src/lib/image.ts +++ b/packages/shared/src/lib/image.ts @@ -16,12 +16,8 @@ export const isImageUrl = ( const parsedUrl = new URL(url, baseUrl); const host = parsedUrl.hostname.toLowerCase(); - if (host === DAILY_MEDIA_HOST) { - return true; - } - if ( - CLOUDINARY_IMAGE_HOST_PATTERN.test(host) && + (host === DAILY_MEDIA_HOST || CLOUDINARY_IMAGE_HOST_PATTERN.test(host)) && parsedUrl.pathname.includes('/image/') ) { return true;