Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add server side validation for uploaded file types #173960

Merged
merged 13 commits into from Jan 30, 2024
Merged

Add server side validation for uploaded file types #173960

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
82e4a88
Add server side validation for uploaded file types to image url
SiddharthMantri Dec 26, 2023
fca7724
use correct response code for unsupported media type
SiddharthMantri Dec 28, 2023
59fee3d
Merge branch 'main' into serverside-file-validation
SiddharthMantri Dec 28, 2023
ef42f09
update imports and remove export for image file type constant
SiddharthMantri Dec 29, 2023
e60693e
Merge branch 'main' into serverside-file-validation
SiddharthMantri Jan 10, 2024
a7f3f10
Merge branch 'main' into serverside-file-validation
SiddharthMantri Jan 11, 2024
8125ead
Merge branch 'main' into serverside-file-validation
SiddharthMantri Jan 12, 2024
a02b323
Use valid base64 string in tests and update endpoint check
SiddharthMantri Jan 12, 2024
63cb71b
Use test image data url for trial and basic tests
SiddharthMantri Jan 12, 2024
d52c6cf
Merge branch 'main' into serverside-file-validation
SiddharthMantri Jan 12, 2024
0f64429
Update tests and use test results to fix code
SiddharthMantri Jan 13, 2024
43cc864
Merge branch 'main' into serverside-file-validation
SiddharthMantri Jan 17, 2024
c8578d6
Merge branch 'main' into serverside-file-validation
kibanamachine Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions x-pack/plugins/security/common/constants.ts
View file
Open in desktop
Expand Up @@ -92,3 +92,8 @@ export const SESSION_EXTENSION_THROTTLE_MS = 60 * 1000;
* Route to get session info and extend session expiration
*/
export const SESSION_ROUTE = '/internal/security/session';

/**
* Allowed image file types for uploading an image as avatar
*/
export const IMAGE_FILE_TYPES = ['image/svg+xml', 'image/jpeg', 'image/png', 'image/gif'];
4 changes: 3 additions & 1 deletion x-pack/plugins/security/public/account_management/user_profile/utils.ts
View file
Open in desktop
Expand Up @@ -4,8 +4,10 @@
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import { IMAGE_FILE_TYPES } from '../../../common/constants';

export { IMAGE_FILE_TYPES } from '../../../common/constants';
SiddharthMantri marked this conversation as resolved.
Show resolved Hide resolved

export const IMAGE_FILE_TYPES = ['image/svg+xml', 'image/jpeg', 'image/png', 'image/gif'];
export const MAX_IMAGE_SIZE = 64;

export function readFile(data: File) {
Expand Down
20 changes: 20 additions & 0 deletions x-pack/plugins/security/server/routes/user_profile/update.ts
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@
import { schema } from '@kbn/config-schema';

import type { RouteDefinitionParams } from '..';
import { IMAGE_FILE_TYPES } from '../../../common/constants';
import { wrapIntoCustomErrorResponse } from '../../errors';
import { flattenObject } from '../../lib';
import { getPrintableSessionId } from '../../session_management';
Expand Down Expand Up @@ -47,7 +48,26 @@ export function defineUpdateUserProfileDataRoute({
}

const currentUser = getAuthenticationService().getCurrentUser(request);

const userProfileData = request.body;
const imageDataUrl = userProfileData.avatar.imageUrl;
const matches = imageDataUrl.match(/^data:([A-Za-z-+\/]+);base64,(.+)$/);
if (!matches || matches.length !== 3) {
return response.customError({
body: 'Unsupported media type',
statusCode: 415,
});
}

const [, mimeType] = matches;

if (!IMAGE_FILE_TYPES.includes(mimeType)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also constrain the image size (automatically resize) as we do via the UI?
https://github.com/elastic/kibana/blob/079db3e7ebcbed934c39d0efe67821317fe9eaec/x-pack/plugins/security/public/account_management/user_profile/utils.ts#L63C8-L63C8

I did notice that at some point the payload max size kicks in, which would limit the incoming image size, but not to same degree that the UI resizes.

{
"statusCode": 413,
"error": "Request Entity Too Large",
"message": "Payload content length greater than maximum allowed: 1048576"
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can see that if I upload via API, the image is much more clear because it is not resized:
Screenshot 2023-12-28 at 1 24 26 PM

But if I upload via the UI, it gets resized:
Screenshot 2023-12-28 at 1 24 02 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! As image resizing is done via the Image and Canvas APIs on the browser, we'll need to use libraries to do the same for the node server which means adding a new dependency. Where does this dependency go? Top level package.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a separate issue for this. https://github.com/elastic/security/issues/1868

return response.customError({
body: 'Unsupported media type',
statusCode: 415,
});
}

const keysToUpdate = Object.keys(flattenObject(userProfileData));

if (currentUser?.elastic_cloud_user) {
Expand Down