fix: attachments with # in the filename fail to download

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);
+	});
+});

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;
 };

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<string, unknown> = 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<string, unknown>) => {
 		const { Text } = require('react-native');
+		mockImageViewer(props);
 		return <Text>Image Viewer</Text>;
 	}
 }));
@@ -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(<AttachmentView />);
+			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);
+		});
+	});
 });

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 (
 			<ImageViewer
 				uri={uri}
@@ -74,8 +74,7 @@ const RenderContent = ({
 		);
 	}
 	if (attachment.video_url) {
-		const url = formatAttachmentUrl(attachment.title_link || attachment.video_url, user.id, user.token, baseUrl);
-		const uri = encodeURI(url);
+		const uri = formatAttachmentUrl(attachment.title_link || attachment.video_url, user.id, user.token, baseUrl);
 		return (
 			<Video
 				source={{ uri }}