diff --git a/app/lib/methods/helpers/formatAttachmentUrl.test.ts b/app/lib/methods/helpers/formatAttachmentUrl.test.ts new file mode 100644 index 0000000000..3b37f0fac1 --- /dev/null +++ b/app/lib/methods/helpers/formatAttachmentUrl.test.ts @@ -0,0 +1,86 @@ +import { formatAttachmentUrl } from './formatAttachmentUrl'; +import { store as reduxStore } from '../../store/auxStore'; + +jest.mock('../../store/auxStore', () => ({ + store: { + getState: jest.fn() + } +})); + +const mockedGetState = reduxStore.getState as jest.Mock; + +const SERVER = 'https://open.rocket.chat'; +const USER_ID = 'userId'; +const TOKEN = 'token'; + +const mockSettings = ({ protectFiles = false, cdnPrefix = '' } = {}) => + mockedGetState.mockReturnValue({ settings: { FileUpload_ProtectFiles: protectFiles, CDN_PREFIX: cdnPrefix } }); + +describe('formatAttachmentUrl', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + // The server builds the path with `encodeURI(file.name)`, which encodes the spaces but leaves `#` raw — the url + // would otherwise be cut at the fragment and the server would receive `/file-upload/1/a%20video%20`. + describe('raw `#` in the filename', () => { + const rawUrl = '/file-upload/1/a%20video%20#2.mov'; + const escapedUrl = `${SERVER}/file-upload/1/a%20video%20%232.mov`; + + test('escapes it on a relative url', () => { + mockSettings(); + expect(formatAttachmentUrl(rawUrl, USER_ID, TOKEN, SERVER)).toBe(escapedUrl); + }); + + test('escapes it on an absolute url', () => { + mockSettings(); + expect(formatAttachmentUrl(`${SERVER}${rawUrl}`, USER_ID, TOKEN, SERVER)).toBe(escapedUrl); + }); + + test('keeps the whole path when the auth params are appended', () => { + mockSettings({ protectFiles: true }); + expect(formatAttachmentUrl(rawUrl, USER_ID, TOKEN, SERVER)).toBe(`${escapedUrl}?rc_token=${TOKEN}&rc_uid=${USER_ID}`); + }); + + // The `%20`s the server already applied must survive as-is — encoding them again would give `%2520`. + test('escapes it without re-encoding the rest of the path', () => { + mockSettings(); + expect(formatAttachmentUrl(`${SERVER}${rawUrl}?rc_token=${TOKEN}&rc_uid=${USER_ID}`, USER_ID, TOKEN, SERVER)).toBe( + `${escapedUrl}?rc_token=${TOKEN}&rc_uid=${USER_ID}` + ); + }); + + test('escapes it behind a cdn prefix', () => { + mockSettings({ cdnPrefix: 'https://cdn.example.com/' }); + expect(formatAttachmentUrl(rawUrl, USER_ID, TOKEN, SERVER)).toBe( + 'https://cdn.example.com/file-upload/1/a%20video%20%232.mov' + ); + }); + }); + + test('leaves an already-escaped `#` untouched', () => { + mockSettings(); + expect(formatAttachmentUrl('/file-upload/1/a%20video%20%232.mov', USER_ID, TOKEN, SERVER)).toBe( + `${SERVER}/file-upload/1/a%20video%20%232.mov` + ); + }); + + // An external url is not a file-upload path, so a `#` there can be a genuine fragment. + test('returns an external original url verbatim', () => { + mockSettings(); + const externalUrl = 'https://example.com/page#section'; + expect(formatAttachmentUrl(`${SERVER}/file-upload/1/file.mov`, USER_ID, TOKEN, SERVER, externalUrl)).toBe(externalUrl); + }); + + test('returns a base64 data uri untouched', () => { + mockSettings(); + const base64 = 'data:image/png;base64,ABC123'; + expect(formatAttachmentUrl(base64, USER_ID, TOKEN, SERVER)).toBe(base64); + }); + + test('returns a local file uri untouched', () => { + mockSettings(); + const fileUri = 'file:///var/app/Documents/server/msg1/video.mov'; + expect(formatAttachmentUrl(fileUri, USER_ID, TOKEN, SERVER)).toBe(fileUri); + }); +}); diff --git a/app/lib/methods/helpers/formatAttachmentUrl.ts b/app/lib/methods/helpers/formatAttachmentUrl.ts index a4f47b0cc3..6c68bbb321 100644 --- a/app/lib/methods/helpers/formatAttachmentUrl.ts +++ b/app/lib/methods/helpers/formatAttachmentUrl.ts @@ -10,6 +10,9 @@ function setParamInUrl({ url, token, userId }: { url: string; token: string; use return urlObj.toString(); } +// The server encodes the path with `encodeURI(file.name)`, which leaves `#` raw and truncates the url at the fragment. +const escapeFragmentDelimiter = (url: string) => url.replace(/#/g, '%23'); + export const formatAttachmentUrl = ( attachmentUrl: string | undefined, userId: string, @@ -27,18 +30,21 @@ export const formatAttachmentUrl = ( return _originalUrl; } - if (attachmentUrl.includes('rc_token')) { - return encodeURI(attachmentUrl); + const url = escapeFragmentDelimiter(attachmentUrl); + + if (url.includes('rc_token')) { + return url; } - if (protectFiles) return setParamInUrl({ url: attachmentUrl, token, userId }); - return attachmentUrl; + if (protectFiles) return setParamInUrl({ url, token, userId }); + return url; } let cdnPrefix = store?.getState().settings.CDN_PREFIX as string; cdnPrefix = cdnPrefix?.trim(); if (cdnPrefix && cdnPrefix.startsWith('http')) { server = cdnPrefix.replace(/\/+$/, ''); } - if (protectFiles) return setParamInUrl({ url: `${server}${attachmentUrl}`, token, userId }); - return `${server}${attachmentUrl}`; + const url = escapeFragmentDelimiter(`${server}${attachmentUrl}`); + if (protectFiles) return setParamInUrl({ url, token, userId }); + return url; }; diff --git a/app/views/AttachmentView.test.tsx b/app/views/AttachmentView.test.tsx index 0376332a13..7abf7cfc37 100644 --- a/app/views/AttachmentView.test.tsx +++ b/app/views/AttachmentView.test.tsx @@ -9,6 +9,14 @@ const mockNavigation = { pop: jest.fn() }; const mockUseAltTextSupported = jest.fn(); +const mockImageViewer = jest.fn(); + +const DEFAULT_ATTACHMENT = { + title: 'IMG_2444.jpg', + image_url: 'https://open.rocket.chat/image.png', + description: 'A wavy orange and black pattern' +}; +let mockAttachment: Record = DEFAULT_ATTACHMENT; jest.mock('@react-navigation/elements', () => ({ useHeaderHeight: () => 0 @@ -42,8 +50,9 @@ jest.mock('../containers/ActionSheet', () => ({ jest.mock('../containers/ActivityIndicator', () => () => null); jest.mock('../containers/ImageViewer', () => ({ - ImageViewer: () => { + ImageViewer: (props: Record) => { const { Text } = require('react-native'); + mockImageViewer(props); return Image Viewer; } })); @@ -67,11 +76,7 @@ jest.mock('../lib/hooks/navigation', () => ({ useAppNavigation: () => mockNavigation, useAppRoute: () => ({ params: { - attachment: { - title: 'IMG_2444.jpg', - image_url: 'https://open.rocket.chat/image.png', - description: 'A wavy orange and black pattern' - } + attachment: mockAttachment } }) })); @@ -104,6 +109,7 @@ jest.mock('../lib/methods/helpers', () => ({ describe('AttachmentView', () => { beforeEach(() => { jest.clearAllMocks(); + mockAttachment = DEFAULT_ATTACHMENT; }); it('renders the alt text label and opens the action sheet when pressed', () => { @@ -124,4 +130,34 @@ describe('AttachmentView', () => { expect(queryByTestId('attachment-view-alt-text-label')).toBeNull(); }); + + describe('gif detection from the url', () => { + const renderWithImageUrl = (image_url: string) => { + mockUseAltTextSupported.mockReturnValue(false); + mockAttachment = { title: 'clip.gif', image_url }; + render(); + return mockImageViewer.mock.calls[0][0].isAnimated; + }; + + test('detects a plain .gif', () => { + expect(renderWithImageUrl('https://open.rocket.chat/file-upload/1/clip.gif')).toBe(true); + }); + + test('detects a .gif followed by a query', () => { + expect(renderWithImageUrl('https://open.rocket.chat/file-upload/1/clip.gif?rc_token=token')).toBe(true); + }); + + // A `#` in the filename reaches here escaped, so the extension is followed by `%23` rather than `#`. + test('detects a .gif followed by an escaped `#`', () => { + expect(renderWithImageUrl('https://open.rocket.chat/file-upload/1/clip.gif%23draft')).toBe(true); + }); + + test('detects a .gif followed by a raw `#`', () => { + expect(renderWithImageUrl('https://open.rocket.chat/file-upload/1/clip.gif#draft')).toBe(true); + }); + + test('does not flag a non-gif url', () => { + expect(renderWithImageUrl('https://open.rocket.chat/file-upload/1/photo.png')).toBe(false); + }); + }); }); diff --git a/app/views/AttachmentView.tsx b/app/views/AttachmentView.tsx index 322b659ead..78e7b3c355 100644 --- a/app/views/AttachmentView.tsx +++ b/app/views/AttachmentView.tsx @@ -59,9 +59,9 @@ const RenderContent = ({ }, [navigation]); if (attachment.image_url) { - const url = formatAttachmentUrl(attachment.title_link || attachment.image_url, user.id, user.token, baseUrl); - const uri = encodeURI(url); - const isAnimated = attachment.image_type === 'image/gif' || /\.gif(\?|$)/i.test(url); + const uri = formatAttachmentUrl(attachment.title_link || attachment.image_url, user.id, user.token, baseUrl); + // `#` is escaped to `%23` before it reaches here, so both spellings can follow the extension. + const isAnimated = attachment.image_type === 'image/gif' || /\.gif(\?|#|%23|$)/i.test(uri); return (