From a5fa0c1d5c4ef232ce3058e1d323deb9318064ea Mon Sep 17 00:00:00 2001 From: OtavioStasiak Date: Mon, 10 Aug 2026 16:40:17 -0300 Subject: [PATCH 1/4] fix: attachments with # in the filename fail to download --- app/lib/methods/helpers/formatAttachmentUrl.ts | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/app/lib/methods/helpers/formatAttachmentUrl.ts b/app/lib/methods/helpers/formatAttachmentUrl.ts index a4f47b0cc35..09f7a03ce8a 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; } + // encodeURI leaves `#` raw too, so escape after it rather than feeding it an already-escaped url. if (attachmentUrl.includes('rc_token')) { - return encodeURI(attachmentUrl); + return escapeFragmentDelimiter(encodeURI(attachmentUrl)); } - if (protectFiles) return setParamInUrl({ url: attachmentUrl, token, userId }); - return attachmentUrl; + const url = escapeFragmentDelimiter(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; }; From d8a9e68e95a22e5f46cea7de6e11e9a90a03370e Mon Sep 17 00:00:00 2001 From: OtavioStasiak Date: Mon, 10 Aug 2026 16:40:29 -0300 Subject: [PATCH 2/4] fix: test --- .../helpers/formatAttachmentUrl.test.ts | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 app/lib/methods/helpers/formatAttachmentUrl.test.ts diff --git a/app/lib/methods/helpers/formatAttachmentUrl.test.ts b/app/lib/methods/helpers/formatAttachmentUrl.test.ts new file mode 100644 index 00000000000..713f5a0d538 --- /dev/null +++ b/app/lib/methods/helpers/formatAttachmentUrl.test.ts @@ -0,0 +1,85 @@ +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 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}`); + }); + + test('escapes it on a url that already carries the auth params', () => { + mockSettings(); + expect( + formatAttachmentUrl(`${SERVER}/file-upload/1/video#2.mov?rc_token=${TOKEN}&rc_uid=${USER_ID}`, USER_ID, TOKEN, SERVER) + ).toBe(`${SERVER}/file-upload/1/video%232.mov?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); + }); +}); From 88e5f9161848e9628806ea62e30c6b7071a37bc4 Mon Sep 17 00:00:00 2001 From: OtavioStasiak Date: Mon, 10 Aug 2026 16:49:44 -0300 Subject: [PATCH 3/4] fix: broken attachment urls from a raw `#` and double-encoding --- app/lib/methods/helpers/formatAttachmentUrl.test.ts | 13 +++++++------ app/lib/methods/helpers/formatAttachmentUrl.ts | 8 ++++---- app/views/AttachmentView.tsx | 8 +++----- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/app/lib/methods/helpers/formatAttachmentUrl.test.ts b/app/lib/methods/helpers/formatAttachmentUrl.test.ts index 713f5a0d538..3b37f0fac14 100644 --- a/app/lib/methods/helpers/formatAttachmentUrl.test.ts +++ b/app/lib/methods/helpers/formatAttachmentUrl.test.ts @@ -21,8 +21,8 @@ describe('formatAttachmentUrl', () => { jest.clearAllMocks(); }); - // The server builds the path with `encodeURI(file.name)`, which leaves `#` raw — the url would otherwise be cut - // at the fragment and the server would receive `/file-upload/1/a%20video%20`. + // 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`; @@ -42,11 +42,12 @@ describe('formatAttachmentUrl', () => { expect(formatAttachmentUrl(rawUrl, USER_ID, TOKEN, SERVER)).toBe(`${escapedUrl}?rc_token=${TOKEN}&rc_uid=${USER_ID}`); }); - test('escapes it on a url that already carries the auth params', () => { + // 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}/file-upload/1/video#2.mov?rc_token=${TOKEN}&rc_uid=${USER_ID}`, USER_ID, TOKEN, SERVER) - ).toBe(`${SERVER}/file-upload/1/video%232.mov?rc_token=${TOKEN}&rc_uid=${USER_ID}`); + 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', () => { diff --git a/app/lib/methods/helpers/formatAttachmentUrl.ts b/app/lib/methods/helpers/formatAttachmentUrl.ts index 09f7a03ce8a..6c68bbb321b 100644 --- a/app/lib/methods/helpers/formatAttachmentUrl.ts +++ b/app/lib/methods/helpers/formatAttachmentUrl.ts @@ -30,12 +30,12 @@ export const formatAttachmentUrl = ( return _originalUrl; } - // encodeURI leaves `#` raw too, so escape after it rather than feeding it an already-escaped url. - if (attachmentUrl.includes('rc_token')) { - return escapeFragmentDelimiter(encodeURI(attachmentUrl)); + const url = escapeFragmentDelimiter(attachmentUrl); + + if (url.includes('rc_token')) { + return url; } - const url = escapeFragmentDelimiter(attachmentUrl); if (protectFiles) return setParamInUrl({ url, token, userId }); return url; } diff --git a/app/views/AttachmentView.tsx b/app/views/AttachmentView.tsx index 322b659eada..b9a0856aa3e 100644 --- a/app/views/AttachmentView.tsx +++ b/app/views/AttachmentView.tsx @@ -59,9 +59,8 @@ 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); + const isAnimated = attachment.image_type === 'image/gif' || /\.gif(\?|$)/i.test(uri); return ( Date: Mon, 10 Aug 2026 17:27:46 -0300 Subject: [PATCH 4/4] fix: detect gifs when #or%23 follows the extension --- app/views/AttachmentView.test.tsx | 48 +++++++++++++++++++++++++++---- app/views/AttachmentView.tsx | 3 +- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/app/views/AttachmentView.test.tsx b/app/views/AttachmentView.test.tsx index 0376332a134..7abf7cfc374 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 b9a0856aa3e..78e7b3c3554 100644 --- a/app/views/AttachmentView.tsx +++ b/app/views/AttachmentView.tsx @@ -60,7 +60,8 @@ const RenderContent = ({ if (attachment.image_url) { const uri = formatAttachmentUrl(attachment.title_link || attachment.image_url, user.id, user.token, baseUrl); - const isAnimated = attachment.image_type === 'image/gif' || /\.gif(\?|$)/i.test(uri); + // `#` 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 (