diff --git a/app/containers/message/components/Attachments/Image/Image.tsx b/app/containers/message/components/Attachments/Image/Image.tsx index 381ac09e07a..3795e41448c 100644 --- a/app/containers/message/components/Attachments/Image/Image.tsx +++ b/app/containers/message/components/Attachments/Image/Image.tsx @@ -12,6 +12,7 @@ import { useUserPreferences } from '../../../../../lib/methods/userPreferences'; import { AUTOPLAY_GIFS_PREFERENCES_KEY } from '../../../../../lib/constants/keys'; import ImageBadge from './ImageBadge'; import log from '../../../../../lib/methods/helpers/log'; +import { encodeAttachmentUrl } from '../../../../../lib/methods/helpers/formatAttachmentUrl'; export const MessageImage = ({ uri, status, encrypted = false, imagePreview, imageType }: IMessageImage) => { const { colors } = useTheme(); @@ -69,7 +70,7 @@ export const MessageImage = ({ uri, status, encrypted = false, imagePreview, ima <> {showImage ? ( - + ) : null} {['loading', 'to-download'].includes(status) || (status === 'downloaded' && !showImage) ? ( diff --git a/app/containers/message/hooks/__tests__/useFile.test.ts b/app/containers/message/hooks/__tests__/useFile.test.ts index 373fffe3315..4cd27daff8e 100644 --- a/app/containers/message/hooks/__tests__/useFile.test.ts +++ b/app/containers/message/hooks/__tests__/useFile.test.ts @@ -2,106 +2,47 @@ import { act, renderHook } from '@testing-library/react-native'; import { useFile } from '../useFile'; import { type IAttachment } from '../../../../definitions'; -import { getMessageById } from '../../../../lib/database/services/Message'; -import { getThreadMessageById } from '../../../../lib/database/services/ThreadMessage'; - -jest.mock('../../../../lib/database/services/Message', () => ({ - getMessageById: jest.fn() -})); - -jest.mock('../../../../lib/database/services/ThreadMessage', () => ({ - getThreadMessageById: jest.fn() -})); - -const mockGetMessageById = getMessageById as jest.Mock; -const mockGetThreadMessageById = getThreadMessageById as jest.Mock; const file = { title: 'original.png', title_link: '/original' } as IAttachment; -// Flushes the async checkMessage effect (two awaited DB reads) inside act so any -// setIsMessagePersisted update settles before assertions run. -const flushEffect = () => - act(async () => { - await Promise.resolve(); - await Promise.resolve(); - }); - describe('useFile', () => { - beforeEach(() => { - jest.clearAllMocks(); - mockGetThreadMessageById.mockResolvedValue(undefined); - mockGetMessageById.mockResolvedValue(undefined); - }); - - it('returns the file prop and treats forwarded merges as a no-op while the thread message persists', async () => { - mockGetThreadMessageById.mockResolvedValue({ id: 'thread-message' }); - const { result } = renderHook(() => useFile(file, 'msg-id')); - - await flushEffect(); - expect(mockGetThreadMessageById).toHaveBeenCalledWith('msg-id'); - expect(mockGetMessageById).not.toHaveBeenCalled(); - expect(result.current[0]).toBe(file); - - act(() => result.current[1]({ title_link: '/forwarded' })); - expect(result.current[0]).toBe(file); - }); - - it('stays persisted when no thread message exists but the message is found', async () => { - mockGetThreadMessageById.mockResolvedValue(undefined); - mockGetMessageById.mockResolvedValue({ id: 'message' }); - const { result } = renderHook(() => useFile(file, 'msg-id')); - - await flushEffect(); - expect(mockGetMessageById).toHaveBeenCalledWith('msg-id'); - expect(result.current[0]).toBe(file); + it('returns the file prop untouched until something is merged', () => { + const { result } = renderHook(() => useFile(file)); - act(() => result.current[1]({ title_link: '/forwarded' })); expect(result.current[0]).toBe(file); }); - it('becomes not-persisted and returns the merged localFile when neither message is found', async () => { - const { result } = renderHook(() => useFile(file, 'msg-id')); + it('merges an override on top of the file prop', () => { + const { result } = renderHook(() => useFile(file)); - await flushEffect(); - expect(mockGetMessageById).toHaveBeenCalledWith('msg-id'); + act(() => result.current[1]({ title_link: 'file:///local/original.png' })); - act(() => result.current[1]({ title_link: '/forwarded' })); - expect(result.current[0].title_link).toBe('/forwarded'); expect(result.current[0]).not.toBe(file); + expect(result.current[0].title_link).toBe('file:///local/original.png'); expect(result.current[0].title).toBe('original.png'); }); - it('starts not-persisted when messageId is empty and merges forwarded files into localFile', async () => { - const { result } = renderHook(() => useFile(file, '')); + it('accumulates successive overrides', () => { + const { result } = renderHook(() => useFile(file)); - await flushEffect(); - expect(result.current[0]).toBe(file); + act(() => result.current[1]({ title_link: 'file:///local/original.png' })); + act(() => result.current[1]({ e2e: 'done' })); - act(() => result.current[1]({ title_link: '/forwarded' })); - expect(result.current[0]).not.toBe(file); - expect(result.current[0].title_link).toBe('/forwarded'); + expect(result.current[0].title_link).toBe('file:///local/original.png'); + expect(result.current[0].e2e).toBe('done'); }); - // Documents the current contract: isMessagePersisted is seeded once and the effect only ever - // flips it to false, so it can't recover to true on a later messageId change. Not fixed: the - // only caller keys its list by that same id, so a real id change always remounts the hook fresh. - it('stays stuck not-persisted after rerendering with a real persisted messageId (documents current contract)', async () => { - mockGetThreadMessageById.mockResolvedValue(undefined); - mockGetMessageById.mockResolvedValue(undefined); - const { result, rerender } = renderHook(({ messageId }: { messageId: string }) => useFile(file, messageId), { - initialProps: { messageId: '' } + // The override is what the download resolved to locally, so it has to win over a later `file` + // prop that still carries the remote url — that stale prop is exactly the bug this hook fixes. + it('keeps overrides applied when the file prop changes', () => { + const { result, rerender } = renderHook(({ f }: { f: IAttachment }) => useFile(f), { + initialProps: { f: file } }); - await flushEffect(); - expect(result.current[0]).toBe(file); + act(() => result.current[1]({ title_link: 'file:///local/original.png' })); + rerender({ f: { ...file, title: 'renamed.png' } as IAttachment }); - mockGetMessageById.mockResolvedValue({ id: 'msg-id' }); - rerender({ messageId: 'msg-id' }); - await flushEffect(); - - expect(mockGetMessageById).toHaveBeenCalledWith('msg-id'); - act(() => result.current[1]({ title_link: '/forwarded' })); - expect(result.current[0]).not.toBe(file); - expect(result.current[0].title_link).toBe('/forwarded'); + expect(result.current[0].title).toBe('renamed.png'); + expect(result.current[0].title_link).toBe('file:///local/original.png'); }); }); diff --git a/app/containers/message/hooks/useFile.tsx b/app/containers/message/hooks/useFile.tsx index e21898e4b45..6dcb1438e9b 100644 --- a/app/containers/message/hooks/useFile.tsx +++ b/app/containers/message/hooks/useFile.tsx @@ -1,30 +1,13 @@ -import { useEffect, useState } from 'react'; +import { useState } from 'react'; import { type IAttachment } from '../../../definitions'; -import { getMessageById } from '../../../lib/database/services/Message'; -import { getThreadMessageById } from '../../../lib/database/services/ThreadMessage'; -export const useFile = (file: IAttachment, messageId: string) => { - const [localFile, setLocalFile] = useState(file); - const [isMessagePersisted, setIsMessagePersisted] = useState(!!messageId); - useEffect(() => { - const checkMessage = async () => { - const threadMessage = await getThreadMessageById(messageId); - if (!threadMessage) { - const message = await getMessageById(messageId); - if (!message) { - setIsMessagePersisted(false); - } - } - }; - checkMessage(); - }, [messageId]); +export const useFile = (file: IAttachment) => { + const [overrides, setOverrides] = useState | null>(null); - const manageForwardedFile = (f: Partial) => { - if (isMessagePersisted) { - return; - } - setLocalFile(prev => ({ ...prev, ...f })); + const mergeFile = (f: Partial) => { + setOverrides(prev => ({ ...prev, ...f })); }; - return [isMessagePersisted ? file : localFile, manageForwardedFile] as const; + + return [overrides ? { ...file, ...overrides } : file, mergeFile] as const; }; diff --git a/app/containers/message/hooks/useMediaAutoDownload.tsx b/app/containers/message/hooks/useMediaAutoDownload.tsx index ac17bf25001..ec146e54fde 100644 --- a/app/containers/message/hooks/useMediaAutoDownload.tsx +++ b/app/containers/message/hooks/useMediaAutoDownload.tsx @@ -80,7 +80,7 @@ export const useMediaAutoDownload = ({ const baseUrl = useBaseUrl(); const user = useMessageUser(); const [status, dispatchDownloadEvent] = useReducer(downloadStatusReducer, 'to-download'); - const [currentFile, setCurrentFile] = useFile(file, id ?? ''); + const [currentFile, setCurrentFile] = useFile(file); const originalUrl = getOriginalURL(file); const url = formatAttachmentUrl( file.title_link || getFileProperty(currentFile, fileType, 'url'), diff --git a/app/lib/methods/handleMediaDownload.test.ts b/app/lib/methods/handleMediaDownload.test.ts index 29fafaa4f01..0035df697a6 100644 --- a/app/lib/methods/handleMediaDownload.test.ts +++ b/app/lib/methods/handleMediaDownload.test.ts @@ -1,4 +1,4 @@ -import { getFilename, matchDownloadUrl, persistMessage } from './handleMediaDownload'; +import { getFilePath, getFilename, matchDownloadUrl, persistMessage } from './handleMediaDownload'; import database from '../database'; import { getMessageById } from '../database/services/Message'; import { getThreadById } from '../database/services/Thread'; @@ -33,6 +33,22 @@ jest.mock('../database/services/ThreadMessage', () => ({ getThreadMessageById: jest.fn() })); +jest.mock('../store/auxStore', () => ({ + store: { getState: () => ({ server: { server: 'https://server.com' } }) } +})); + +describe('getFilePath', () => { + it('derives the cache filename from the unencoded url', () => { + expect( + getFilePath({ + type: 'video', + mimeType: 'video/quicktime', + urlToCache: 'https://server.com/file-upload/abc/Screen Recording.mov' + }) + ).toContain('/Screen_Recording.mov'); + }); +}); + describe('matchDownloadUrl', () => { it('matches when downloadUrl contains image_url', () => { expect( @@ -67,6 +83,15 @@ describe('matchDownloadUrl', () => { matchDownloadUrl({ image_url: '/file-upload/abc/photo.jpg' }, 'https://server.com/file-upload/abc/audio.mp3') ).toBeFalsy(); }); + + it('matches an attachment url with a space against the download url', () => { + expect( + matchDownloadUrl( + { video_url: '/file-upload/abc/Screen Recording.mov' }, + 'https://server.com/file-upload/abc/Screen Recording.mov' + ) + ).toBeTruthy(); + }); }); describe('Test the getFilename', () => { diff --git a/app/lib/methods/helpers/__tests__/formatAttachmentUrl.test.ts b/app/lib/methods/helpers/__tests__/formatAttachmentUrl.test.ts new file mode 100644 index 00000000000..10e08b1836c --- /dev/null +++ b/app/lib/methods/helpers/__tests__/formatAttachmentUrl.test.ts @@ -0,0 +1,61 @@ +import { encodeAttachmentUrl } from '../formatAttachmentUrl'; + +describe('encodeAttachmentUrl', () => { + it('encodes an unencoded path', () => { + expect(encodeAttachmentUrl('https://open.rocket.chat/file-upload/1/Screen Recording.mov')).toBe( + 'https://open.rocket.chat/file-upload/1/Screen%20Recording.mov' + ); + }); + + it('leaves an already-encoded path untouched', () => { + expect(encodeAttachmentUrl('https://open.rocket.chat/file-upload/1/Screen%20Recording.mov')).toBe( + 'https://open.rocket.chat/file-upload/1/Screen%20Recording.mov' + ); + }); + + it('leaves an already-escaped reserved character in the path', () => { + expect(encodeAttachmentUrl('https://open.rocket.chat/file-upload/1/a%20video%20%232.mov')).toBe( + 'https://open.rocket.chat/file-upload/1/a%20video%20%232.mov' + ); + }); + + // Known limitation, not a regression: `#` is the fragment delimiter per the URL spec, so a raw one ends the + // path and the rest becomes the fragment — which HTTP drops, so the server sees a truncated path. The previous + // encodeURI behaved identically (it leaves `#` unescaped too). Only reachable if the server sends a raw `#`. + it('treats a raw reserved `#` in the path as a fragment', () => { + expect(encodeAttachmentUrl('https://open.rocket.chat/file-upload/1/a video #2.mov')).toBe( + 'https://open.rocket.chat/file-upload/1/a%20video%20#2.mov' + ); + }); + + it('preserves the query string', () => { + expect(encodeAttachmentUrl('https://open.rocket.chat/file-upload/1/Screen Recording.mov?rc_token=abc&rc_uid=123')).toBe( + 'https://open.rocket.chat/file-upload/1/Screen%20Recording.mov?rc_token=abc&rc_uid=123' + ); + }); + + // WHATWG URL passes a malformed escape through rather than throwing, so this exercises the try branch. + it('returns the raw url when it has a malformed escape', () => { + expect(encodeAttachmentUrl('https://open.rocket.chat/file-upload/1/%ZZ.mov')).toBe( + 'https://open.rocket.chat/file-upload/1/%ZZ.mov' + ); + }); + + // A non-absolute url is what actually throws — reachable when the server/CDN prefix is empty. + it('returns the raw url when it is not absolute', () => { + expect(encodeAttachmentUrl('/file-upload/1/Screen Recording.mov')).toBe('/file-upload/1/Screen Recording.mov'); + }); + + // Cache filenames keep unicode letters, so local uris need encoding before they reach the native players. + it('encodes unicode letters in a local file uri', () => { + expect(encodeAttachmentUrl('file:///var/app/Documents/server/msg1/vídeo.mov')).toBe( + 'file:///var/app/Documents/server/msg1/v%C3%ADdeo.mov' + ); + }); + + it('leaves an already-encoded local file uri untouched', () => { + expect(encodeAttachmentUrl('file:///var/app/Documents/server/msg1/v%C3%ADdeo.mov')).toBe( + 'file:///var/app/Documents/server/msg1/v%C3%ADdeo.mov' + ); + }); +}); diff --git a/app/lib/methods/helpers/formatAttachmentUrl.ts b/app/lib/methods/helpers/formatAttachmentUrl.ts index a4f47b0cc35..217fdd1bb42 100644 --- a/app/lib/methods/helpers/formatAttachmentUrl.ts +++ b/app/lib/methods/helpers/formatAttachmentUrl.ts @@ -10,6 +10,14 @@ function setParamInUrl({ url, token, userId }: { url: string; token: string; use return urlObj.toString(); } +export const encodeAttachmentUrl = (url: string): string => { + try { + return new URL(url).toString(); + } catch { + return url; + } +}; + export const formatAttachmentUrl = ( attachmentUrl: string | undefined, userId: string, @@ -28,7 +36,7 @@ export const formatAttachmentUrl = ( } if (attachmentUrl.includes('rc_token')) { - return encodeURI(attachmentUrl); + return encodeAttachmentUrl(attachmentUrl); } if (protectFiles) return setParamInUrl({ url: attachmentUrl, token, userId }); diff --git a/app/views/AttachmentView.test.tsx b/app/views/AttachmentView.test.tsx index 0376332a134..04654b45ac1 100644 --- a/app/views/AttachmentView.test.tsx +++ b/app/views/AttachmentView.test.tsx @@ -96,6 +96,7 @@ jest.mock('../lib/hooks/useAppSelector', () => ({ jest.mock('../lib/methods/helpers', () => ({ formatAttachmentUrl: (url: string) => url, + encodeAttachmentUrl: (url: string) => url, isAndroid: false, fileDownload: jest.fn(), showErrorAlert: jest.fn() diff --git a/app/views/AttachmentView.tsx b/app/views/AttachmentView.tsx index 322b659eada..b698b52048d 100644 --- a/app/views/AttachmentView.tsx +++ b/app/views/AttachmentView.tsx @@ -18,7 +18,7 @@ import I18n from '../i18n'; import { useAltTextSupported } from '../lib/hooks/useAltTextSupported'; import { useAppSelector } from '../lib/hooks/useAppSelector'; import { useAppNavigation, useAppRoute } from '../lib/hooks/navigation'; -import { formatAttachmentUrl, isAndroid, fileDownload, showErrorAlert } from '../lib/methods/helpers'; +import { encodeAttachmentUrl, formatAttachmentUrl, isAndroid, fileDownload, showErrorAlert } from '../lib/methods/helpers'; import EventEmitter from '../lib/methods/helpers/events'; import { getUserSelector } from '../selectors/login'; import { type TNavigation } from '../stacks/stackType'; @@ -60,7 +60,7 @@ const RenderContent = ({ if (attachment.image_url) { const url = formatAttachmentUrl(attachment.title_link || attachment.image_url, user.id, user.token, baseUrl); - const uri = encodeURI(url); + const uri = encodeAttachmentUrl(url); const isAnimated = attachment.image_type === 'image/gif' || /\.gif(\?|$)/i.test(url); return (