Skip to content

Commit cd79677

Browse files
ergot-rpergotsejuliajforesti
authored
fix(a11y): Add inline error for UserAvatarEditor's URL input (RocketChat#36656)
Co-authored-by: erik <erik.gothe@fotbollsdata.se> Co-authored-by: juliajforesti <juliajforesti@gmail.com>
1 parent 0f8350d commit cd79677

File tree

6 files changed

+174
-30
lines changed

6 files changed

+174
-30
lines changed

‎apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx‎

Lines changed: 54 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,17 @@
11
import type { IUser, AvatarObject } from '@rocket.chat/core-typings';
2-
import { Box, Button, Avatar, TextInput, IconButton, Label } from '@rocket.chat/fuselage';
2+
import { Box, Button, Avatar, IconButton } from '@rocket.chat/fuselage';
3+
import { Field, FieldLabel, FieldRow, FieldError, TextInput } from '@rocket.chat/fuselage-forms';
34
import { UserAvatar } from '@rocket.chat/ui-avatar';
45
import { useToastMessageDispatch, useSetting } from '@rocket.chat/ui-contexts';
56
import type { ReactElement, ChangeEvent } from 'react';
6-
import { useId, useState, useCallback } from 'react';
7+
import { useState, useCallback } from 'react';
78
import { useTranslation } from 'react-i18next';
89

910
import type { UserAvatarSuggestion } from './UserAvatarSuggestion';
1011
import UserAvatarSuggestions from './UserAvatarSuggestions';
1112
import { readFileAsDataURL } from './readFileAsDataURL';
1213
import { useSingleFileInput } from '../../../hooks/useSingleFileInput';
14+
import { isSafeAvatarUrl } from '../../../lib/utils/isSafeAvatarUrl';
1315
import { isValidImageFormat } from '../../../lib/utils/isValidImageFormat';
1416

1517
type UserAvatarEditorProps = {
@@ -27,8 +29,8 @@ function UserAvatarEditor({ currentUsername, username, setAvatarObj, name, disab
2729
const rotateImages = useSetting('FileUpload_RotateImages');
2830
const [avatarFromUrl, setAvatarFromUrl] = useState('');
2931
const [newAvatarSource, setNewAvatarSource] = useState<string>();
30-
const imageUrlField = useId();
3132
const dispatchToastMessage = useToastMessageDispatch();
33+
const [avatarUrlError, setAvatarUrlError] = useState<string | undefined>(undefined);
3234

3335
const setUploadedPreview = useCallback(
3436
async (file: File, avatarObj: AvatarObject) => {
@@ -48,9 +50,21 @@ function UserAvatarEditor({ currentUsername, username, setAvatarObj, name, disab
4850

4951
const [clickUpload] = useSingleFileInput(setUploadedPreview);
5052

51-
const handleAddUrl = (): void => {
53+
const handleAddUrl = async (): Promise<void> => {
54+
if (!isSafeAvatarUrl(avatarFromUrl)) {
55+
setAvatarUrlError(t('error-invalid-image-url'));
56+
return;
57+
}
58+
59+
if (!(await isValidImageFormat(avatarFromUrl))) {
60+
setAvatarUrlError(t('error-invalid-image-url'));
61+
return;
62+
}
63+
5264
setNewAvatarSource(avatarFromUrl);
5365
setAvatarObj({ avatarUrl: avatarFromUrl });
66+
setAvatarUrlError(undefined);
67+
dispatchToastMessage({ type: 'info', message: t('Avatar_preview_updated') });
5468
};
5569

5670
const clickReset = (): void => {
@@ -61,7 +75,11 @@ function UserAvatarEditor({ currentUsername, username, setAvatarObj, name, disab
6175
const url = newAvatarSource;
6276

6377
const handleAvatarFromUrlChange = (event: ChangeEvent<HTMLInputElement>): void => {
64-
setAvatarFromUrl(event.currentTarget.value);
78+
if (avatarUrlError) {
79+
setAvatarUrlError(undefined);
80+
}
81+
const { value } = event.currentTarget;
82+
setAvatarFromUrl(value);
6583
};
6684

6785
const handleSelectSuggestion = useCallback(
@@ -87,35 +105,45 @@ function UserAvatarEditor({ currentUsername, username, setAvatarObj, name, disab
87105
imageOrientation: rotateImages ? 'from-image' : 'none',
88106
objectFit: 'contain',
89107
}}
90-
onError={() => dispatchToastMessage({ type: 'error', message: t('error-invalid-image-url') })}
108+
onError={() => setAvatarUrlError(t('error-invalid-image-url'))}
91109
/>
92-
<Box display='flex' flexDirection='column' flexGrow='1' justifyContent='space-between' mis={4}>
110+
<Box display='flex' flexDirection='column' flexGrow='1' mis={4}>
93111
<Box display='flex' flexDirection='row' mbs='none'>
94112
<Button square disabled={disabled} mi={4} title={t('Accounts_SetDefaultAvatar')} onClick={clickReset}>
95113
<Avatar url={`/avatar/%40${useFullNameForDefaultAvatar ? name : username}`} />
96114
</Button>
97115
<IconButton icon='upload' secondary disabled={disabled} title={t('Upload')} mi={4} onClick={clickUpload} />
98-
<IconButton
99-
icon='permalink'
100-
secondary
101-
disabled={disabled || !avatarFromUrl}
102-
title={t('Add_URL')}
103-
mi={4}
104-
onClick={handleAddUrl}
105-
/>
106116
<UserAvatarSuggestions disabled={disabled} onSelectOne={handleSelectSuggestion} />
107117
</Box>
108-
<Label htmlFor={imageUrlField} mis={4}>
109-
{t('Use_url_for_avatar')}
110-
</Label>
111-
<TextInput
112-
id={imageUrlField}
113-
flexGrow={0}
114-
placeholder={t('Use_url_for_avatar')}
115-
value={avatarFromUrl}
116-
mis={4}
117-
onChange={handleAvatarFromUrlChange}
118-
/>
118+
<Field pis={4} mbs={12}>
119+
<FieldLabel>{t('Use_url_for_avatar')}</FieldLabel>
120+
<FieldRow>
121+
<TextInput
122+
placeholder={t('Use_url_for_avatar')}
123+
addon={
124+
<IconButton
125+
icon='permalink'
126+
secondary
127+
small
128+
disabled={disabled || !avatarFromUrl || !!avatarUrlError}
129+
title={t('Add_URL')}
130+
onClick={handleAddUrl}
131+
mb={-4}
132+
mie={-4}
133+
/>
134+
}
135+
value={avatarFromUrl}
136+
onChange={handleAvatarFromUrlChange}
137+
error={avatarUrlError}
138+
onKeyDown={(event): void => {
139+
if (event.key === 'Enter') {
140+
handleAddUrl();
141+
}
142+
}}
143+
/>
144+
</FieldRow>
145+
{avatarUrlError && <FieldError>{avatarUrlError}</FieldError>}
146+
</Field>
119147
</Box>
120148
</Box>
121149
</Box>

‎apps/meteor/client/lib/utils/isSafeAvatarUrl.spec.ts‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
import { isSafeAvatarUrl } from './isSafeAvatarUrl';
2+
3+
describe('valid URLs', () => {
4+
it('should accept HTTPS URLs', () => {
5+
expect(isSafeAvatarUrl('https://example.com/avatar.png')).toBe(true);
6+
expect(isSafeAvatarUrl('https://cdn.example.com/user/123/avatar.jpg')).toBe(true);
7+
});
8+
9+
it('should accept HTTP URLs', () => {
10+
expect(isSafeAvatarUrl('http://example.com/avatar.png')).toBe(true);
11+
expect(isSafeAvatarUrl('http://localhost:3000/avatar.png')).toBe(true);
12+
});
13+
14+
it('should accept data URLs with image MIME types', () => {
15+
expect(isSafeAvatarUrl('data:image/png;base64,iVBORw0KGgoAAAANSUhEUg')).toBe(true);
16+
expect(isSafeAvatarUrl('data:image/jpeg;base64,/9j/4AAQSkZJRg')).toBe(true);
17+
expect(isSafeAvatarUrl('data:image/gif;base64,R0lGODlh')).toBe(true);
18+
expect(isSafeAvatarUrl('data:image/webp;base64,UklGRiQA')).toBe(true);
19+
expect(isSafeAvatarUrl('data:image/svg+xml;base64,PHN2Zw')).toBe(true);
20+
});
21+
22+
it('should be case-insensitive for protocols', () => {
23+
expect(isSafeAvatarUrl('HTTPS://example.com/avatar.png')).toBe(true);
24+
expect(isSafeAvatarUrl('HTTP://example.com/avatar.png')).toBe(true);
25+
expect(isSafeAvatarUrl('DATA:IMAGE/PNG;base64,iVBORw0KGgoAAAANSUhEUg')).toBe(true);
26+
});
27+
});
28+
29+
describe('invalid URLs', () => {
30+
it('should reject javascript: protocol', () => {
31+
expect(isSafeAvatarUrl('javascript:alert(1)')).toBe(false);
32+
expect(isSafeAvatarUrl('javascript:void(0)')).toBe(false);
33+
});
34+
35+
it('should reject file: protocol', () => {
36+
expect(isSafeAvatarUrl('file:///etc/passwd')).toBe(false);
37+
expect(isSafeAvatarUrl('file:///C:/Windows/System32')).toBe(false);
38+
});
39+
40+
it('should reject data: URLs with non-image MIME types', () => {
41+
expect(isSafeAvatarUrl('data:text/html;base64,PHNjcmlwdD4')).toBe(false);
42+
expect(isSafeAvatarUrl('data:text/javascript;base64,YWxlcnQoMSk')).toBe(false);
43+
expect(isSafeAvatarUrl('data:application/javascript;base64,YWxlcnQoMSk')).toBe(false);
44+
expect(isSafeAvatarUrl('data:text/plain;base64,SGVsbG8=')).toBe(false);
45+
});
46+
47+
it('should reject ftp: protocol', () => {
48+
expect(isSafeAvatarUrl('ftp://example.com/avatar.png')).toBe(false);
49+
});
50+
51+
it('should reject malformed URLs', () => {
52+
expect(isSafeAvatarUrl('not a url')).toBe(false);
53+
expect(isSafeAvatarUrl('htp://missing-t.com')).toBe(false);
54+
expect(isSafeAvatarUrl('://example.com')).toBe(false);
55+
});
56+
57+
it('should reject empty strings', () => {
58+
expect(isSafeAvatarUrl('')).toBe(false);
59+
});
60+
61+
it('should reject other dangerous protocols', () => {
62+
expect(isSafeAvatarUrl('vbscript:msgbox(1)')).toBe(false);
63+
expect(isSafeAvatarUrl('data:text/html,<script>alert(1)</script>')).toBe(false);
64+
});
65+
});
66+
67+
describe('URL edge cases', () => {
68+
it('should handle URLs with query parameters', () => {
69+
expect(isSafeAvatarUrl('https://example.com/avatar.png?size=large&format=webp')).toBe(true);
70+
});
71+
72+
it('should handle URLs with fragments', () => {
73+
expect(isSafeAvatarUrl('https://example.com/avatar.png#profile')).toBe(true);
74+
});
75+
76+
it('should handle URLs with authentication', () => {
77+
expect(isSafeAvatarUrl('https://user:pass@example.com/avatar.png')).toBe(true);
78+
});
79+
80+
it('should handle URLs with non-standard ports', () => {
81+
expect(isSafeAvatarUrl('http://example.com:8080/avatar.png')).toBe(true);
82+
expect(isSafeAvatarUrl('https://example.com:443/avatar.png')).toBe(true);
83+
});
84+
});

‎apps/meteor/client/lib/utils/isSafeAvatarUrl.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
export function isSafeAvatarUrl(urlString: string): boolean {
2+
try {
3+
const url = new URL(urlString);
4+
const protocol = url.protocol.toLowerCase();
5+
6+
if (protocol === 'http:' || protocol === 'https:') {
7+
return true;
8+
}
9+
if (protocol === 'data:') {
10+
return urlString.toLowerCase().startsWith('data:image/');
11+
}
12+
13+
return false;
14+
} catch {
15+
return false;
16+
}
17+
}

‎apps/meteor/tests/e2e/account-profile.spec.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,18 @@ test.describe.serial('settings-account-profile', () => {
5858
await expect(poAccountProfile.userAvatarEditor).toHaveAttribute('src');
5959
});
6060

61-
test('should display a skeleton if the image url is not valid', async () => {
61+
test('should show inline error if the image url is not valid', async () => {
6262
await poAccountProfile.inputAvatarLink.fill('https://invalidUrl');
6363
await poAccountProfile.btnSetAvatarLink.click();
6464

65-
await poAccountProfile.btnSubmit.click();
66-
await expect(poAccountProfile.userAvatarEditor).not.toHaveAttribute('src');
65+
await expect(poAccountProfile.errorInvalidUrl).toBeVisible();
66+
});
67+
68+
test('should show inline error if url does not point to an image', async () => {
69+
await poAccountProfile.inputAvatarLink.fill('https://google.com');
70+
await poAccountProfile.btnSetAvatarLink.click();
71+
72+
await expect(poAccountProfile.errorInvalidUrl).toBeVisible();
6773
});
6874
});
6975
});

‎apps/meteor/tests/e2e/page-objects/account-profile.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,4 +119,12 @@ export class AccountProfile extends Account {
119119
get btnDeleteMyAccount(): Locator {
120120
return this.page.getByRole('button', { name: 'Delete my account' });
121121
}
122+
123+
private getErrorAlertByText(text: string): Locator {
124+
return this.page.getByRole('alert').filter({ hasText: text });
125+
}
126+
127+
get errorInvalidUrl(): Locator {
128+
return this.getErrorAlertByText('Invalid image URL');
129+
}
122130
}

‎packages/i18n/src/locales/en.i18n.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7162,5 +7162,6 @@
71627162
"message_body": "message body",
71637163
"message_attachment": "message attachment",
71647164
"system_message_body": "system message body",
7165-
"No_changes_to_save": "No changes to save"
7165+
"No_changes_to_save": "No changes to save",
7166+
"Avatar_preview_updated": "Avatar preview updated"
71667167
}

0 commit comments

Comments
 (0)