Skip to content

Commit cf5e970

Browse files
[Files Service] Additional validation for mime-type allowances (#234828)
## Summary Summarize your PR. If it involves visual changes include a screenshot or gif. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - ~~[ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)~~ - ~~[ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials~~ - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - ~~[ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)~~ - ~~[ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations.~~ - ~~[ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed~~~ - ~~[ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)~~ - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ... --------- Co-authored-by: kibanamachine <[email protected]>
1 parent 28ce557 commit cf5e970

File tree

12 files changed

+749
-69
lines changed

12 files changed

+749
-69
lines changed

src/platform/plugins/shared/files/server/routes/common.test.ts

Lines changed: 12 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -8,43 +8,29 @@
88
*/
99

1010
import type { File } from '../file';
11-
import { getDownloadHeadersForFile } from './common';
11+
import { getFileHttpResponseOptions } from './common';
1212

1313
describe('getDownloadHeadersForFile', () => {
14-
function expectHeaders({ contentType }: { contentType: string }) {
14+
function expectResult({ contentType }: { contentType: string }) {
1515
return {
16-
'content-type': contentType,
17-
'cache-control': 'max-age=31536000, immutable',
16+
fileContentType: contentType,
17+
headers: {
18+
'cache-control': 'max-age=31536000, immutable',
19+
},
1820
};
1921
}
2022

2123
const file = { data: { name: 'test', mimeType: undefined } } as unknown as File;
22-
test('no mime type and name from file object', () => {
23-
expect(getDownloadHeadersForFile({ file, fileName: undefined })).toEqual(
24-
expectHeaders({ contentType: 'application/octet-stream' })
24+
test('no mime type', () => {
25+
expect(getFileHttpResponseOptions(file)).toEqual(
26+
expectResult({ contentType: 'application/octet-stream' })
2527
);
2628
});
2729

28-
test('no mime type and name (without ext)', () => {
29-
expect(getDownloadHeadersForFile({ file, fileName: 'myfile' })).toEqual(
30-
expectHeaders({ contentType: 'application/octet-stream' })
31-
);
32-
});
33-
test('no mime type and name (with ext)', () => {
34-
expect(getDownloadHeadersForFile({ file, fileName: 'myfile.png' })).toEqual(
35-
expectHeaders({ contentType: 'image/png' })
36-
);
37-
});
38-
test('mime type and no name', () => {
39-
const fileWithMime = { data: { ...file.data, mimeType: 'application/pdf' } } as File;
40-
expect(getDownloadHeadersForFile({ file: fileWithMime, fileName: undefined })).toEqual(
41-
expectHeaders({ contentType: 'application/pdf' })
42-
);
43-
});
44-
test('mime type and name', () => {
30+
test('mime type', () => {
4531
const fileWithMime = { data: { ...file.data, mimeType: 'application/pdf' } } as File;
46-
expect(getDownloadHeadersForFile({ file: fileWithMime, fileName: 'a cool file.pdf' })).toEqual(
47-
expectHeaders({ contentType: 'application/pdf' })
32+
expect(getFileHttpResponseOptions(fileWithMime)).toEqual(
33+
expectResult({ contentType: 'application/pdf' })
4834
);
4935
});
5036
});

src/platform/plugins/shared/files/server/routes/common.ts

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,20 +7,16 @@
77
* License v3.0 only", or the "Server Side Public License, v 1".
88
*/
99

10-
import mime from 'mime';
11-
import type { ResponseHeaders } from '@kbn/core/server';
10+
import type { Readable } from 'stream';
11+
import type { FileHttpResponseOptions } from '@kbn/core-http-server';
1212
import type { File } from '../../common/types';
1313

14-
interface Args {
15-
file: File;
16-
fileName?: string;
17-
}
18-
19-
export function getDownloadHeadersForFile({ file, fileName }: Args): ResponseHeaders {
14+
export function getFileHttpResponseOptions(
15+
file: File
16+
): Pick<FileHttpResponseOptions<Readable>, 'headers' | 'fileContentType'> {
2017
return {
21-
'content-type':
22-
(fileName && mime.getType(fileName)) ?? file.data.mimeType ?? 'application/octet-stream',
23-
'cache-control': 'max-age=31536000, immutable',
18+
fileContentType: file.data.mimeType ?? 'application/octet-stream',
19+
headers: { 'cache-control': 'max-age=31536000, immutable' },
2420
};
2521
}
2622

src/platform/plugins/shared/files/server/routes/file_kind/create.ts

Lines changed: 28 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import type { CreateRouteDefinition } from '../api_routes';
1414
import { FILES_API_ROUTES } from '../api_routes';
1515
import type { FileKindRouter } from './types';
1616
import * as commonSchemas from '../common_schemas';
17+
import { validateMimeType } from './helpers';
1718
import type { CreateHandler } from './types';
1819

1920
export const method = 'post' as const;
@@ -33,25 +34,33 @@ export type Endpoint<M = unknown> = CreateRouteDefinition<
3334
FilesClient['create']
3435
>;
3536

36-
export const handler: CreateHandler<Endpoint> = async ({ core, fileKind, files }, req, res) => {
37-
const [{ security }, { fileService }] = await Promise.all([core, files]);
38-
const {
39-
body: { name, alt, meta, mimeType },
40-
} = req;
41-
const user = security.authc.getCurrentUser();
42-
const file = await fileService.asCurrentUser().create({
43-
fileKind,
44-
name,
45-
alt,
46-
meta,
47-
user: user ? { name: user.username, id: user.profile_uid } : undefined,
48-
mime: mimeType,
49-
});
50-
const body: Endpoint['output'] = {
51-
file: file.toJSON(),
37+
const createHandler =
38+
(fileKindDefinition: FileKind): CreateHandler<Endpoint> =>
39+
async ({ core, fileKind, files }, req, res) => {
40+
const [{ security }, { fileService }] = await Promise.all([core, files]);
41+
const {
42+
body: { name, alt, meta, mimeType },
43+
} = req;
44+
const user = security.authc.getCurrentUser();
45+
46+
const invalidResponse = validateMimeType(mimeType, fileKindDefinition);
47+
if (invalidResponse) {
48+
return invalidResponse;
49+
}
50+
51+
const file = await fileService.asCurrentUser().create({
52+
fileKind,
53+
name,
54+
alt,
55+
meta,
56+
user: user ? { name: user.username, id: user.profile_uid } : undefined,
57+
mime: mimeType,
58+
});
59+
const body: Endpoint['output'] = {
60+
file: file.toJSON(),
61+
};
62+
return res.ok({ body });
5263
};
53-
return res.ok({ body });
54-
};
5564

5665
export function register(fileKindRouter: FileKindRouter, fileKind: FileKind) {
5766
if (fileKind.http.create) {
@@ -67,7 +76,7 @@ export function register(fileKindRouter: FileKindRouter, fileKind: FileKind) {
6776
},
6877
},
6978
},
70-
handler
79+
createHandler(fileKind)
7180
);
7281
}
7382
}

src/platform/plugins/shared/files/server/routes/file_kind/download.ts

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ import type { FilesClient } from '../../../common/files_client';
1313
import type { FileKind } from '../../../common/types';
1414
import { fileNameWithExt } from '../common_schemas';
1515
import { fileErrors } from '../../file';
16-
import { getDownloadHeadersForFile, getDownloadedFileName } from '../common';
17-
import { getById } from './helpers';
16+
import { getFileHttpResponseOptions, getDownloadedFileName } from '../common';
17+
import { getById, validateFileNameExtension } from './helpers';
1818
import type { CreateHandler, FileKindRouter } from './types';
1919
import type { CreateRouteDefinition } from '../api_routes';
2020
import { FILES_API_ROUTES } from '../api_routes';
@@ -37,14 +37,23 @@ export const handler: CreateHandler<Endpoint> = async ({ files, fileKind }, req,
3737
const {
3838
params: { id, fileName },
3939
} = req;
40+
4041
const { error, result: file } = await getById(fileService.asCurrentUser(), id, fileKind);
4142
if (error) return error;
43+
4244
try {
45+
const invalidExtensionResponse = validateFileNameExtension(fileName, file);
46+
if (invalidExtensionResponse) {
47+
return invalidExtensionResponse;
48+
}
49+
4350
const body: Response = await file.downloadContent();
51+
const fileHttpResponseOptions = getFileHttpResponseOptions(file);
52+
4453
return res.file({
4554
body,
4655
filename: fileName ?? getDownloadedFileName(file),
47-
headers: getDownloadHeadersForFile({ file, fileName }),
56+
...fileHttpResponseOptions,
4857
});
4958
} catch (e) {
5059
if (e instanceof fileErrors.NoDownloadAvailableError) {
Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the "Elastic License
4+
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
5+
* Public License v 1"; you may not use this file except in compliance with, at
6+
* your election, the "Elastic License 2.0", the "GNU Affero General Public
7+
* License v3.0 only", or the "Server Side Public License, v 1".
8+
*/
9+
10+
import type { IKibanaResponse } from '@kbn/core/server';
11+
import type { File, FileJSON, FileKind } from '../../../common';
12+
import { validateFileNameExtension, validateMimeType } from './helpers';
13+
14+
describe('helpers', () => {
15+
describe('validateMimeType', () => {
16+
const createFileKind = (allowedMimeTypes?: string[]): FileKind => ({
17+
id: 'test-file-kind',
18+
allowedMimeTypes,
19+
http: {
20+
create: { requiredPrivileges: [] },
21+
download: { requiredPrivileges: [] },
22+
},
23+
});
24+
25+
it('should return undefined when fileKind has empty allowedMimeTypes array', () => {
26+
const fileKind = createFileKind([]);
27+
const result = validateMimeType('image/png', fileKind);
28+
expect(result).toBeUndefined();
29+
});
30+
31+
it('should return undefined when mimeType is in allowedMimeTypes', () => {
32+
const fileKind = createFileKind(['image/png', 'image/jpeg']);
33+
const result = validateMimeType('image/png', fileKind);
34+
expect(result).toBeUndefined();
35+
});
36+
37+
it('should return bad request response when mimeType is not in allowedMimeTypes', () => {
38+
const fileKind = createFileKind(['image/png', 'image/jpeg']);
39+
const result = validateMimeType('application/pdf', fileKind);
40+
41+
expect(result).toBeDefined();
42+
expect((result as IKibanaResponse).status).toBe(400);
43+
expect((result as IKibanaResponse).payload).toEqual({
44+
message: 'File type is not supported',
45+
});
46+
});
47+
48+
it('should be case sensitive for mime type validation', () => {
49+
const fileKind = createFileKind(['image/png']);
50+
const result = validateMimeType('Image/PNG', fileKind);
51+
52+
expect(result).toBeDefined();
53+
expect((result as IKibanaResponse).status).toBe(400);
54+
});
55+
});
56+
57+
describe('validateFileNameExtension', () => {
58+
const createFile = (mimeType?: string) =>
59+
({
60+
id: 'test-file',
61+
data: {
62+
id: 'test-file',
63+
name: 'test-file',
64+
mimeType,
65+
extension: 'txt',
66+
fileKind: 'test',
67+
} as FileJSON,
68+
} as File);
69+
70+
it('should return undefined when fileName is undefined', () => {
71+
const file = createFile('image/png');
72+
const result = validateFileNameExtension(undefined, file);
73+
expect(result).toBeUndefined();
74+
});
75+
76+
it('should return undefined when file is undefined', () => {
77+
const result = validateFileNameExtension('test.png', undefined);
78+
expect(result).toBeUndefined();
79+
});
80+
81+
it('should return undefined when file has no mimeType', () => {
82+
const file = createFile();
83+
const result = validateFileNameExtension('test.png', file);
84+
expect(result).toBeUndefined();
85+
});
86+
87+
it('should return undefined when fileName has no extension', () => {
88+
const file = createFile('text/plain');
89+
const result = validateFileNameExtension('README', file);
90+
expect(result).toBeUndefined();
91+
});
92+
93+
it('should return undefined when extension matches expected extension', () => {
94+
const file = createFile('image/png');
95+
const result = validateFileNameExtension('image.png', file);
96+
expect(result).toBeUndefined();
97+
});
98+
99+
it('should handle mime types with no known extensions', () => {
100+
const file = createFile('application/x-custom-type');
101+
const result = validateFileNameExtension('file.custom', file);
102+
103+
// Should return undefined since there are no expected extensions for this mime type
104+
expect(result).toBeUndefined();
105+
});
106+
107+
it('should handle file names with special characters', () => {
108+
const file = createFile('text/plain');
109+
110+
expect(validateFileNameExtension('[email protected]', file)).toBeUndefined();
111+
expect(validateFileNameExtension('файл.txt', file)).toBeUndefined(); // Unicode filename
112+
expect(validateFileNameExtension('file with spaces.txt', file)).toBeUndefined();
113+
});
114+
115+
it('should trim whitespace from mime type before validation', () => {
116+
const file = createFile(' text/plain ');
117+
const result = validateFileNameExtension('test.txt', file);
118+
expect(result).toBeUndefined();
119+
});
120+
121+
it('should be case insensitive for file extensions', () => {
122+
const file = createFile('image/png');
123+
124+
expect(validateFileNameExtension('image.PNG', file)).toBeUndefined();
125+
expect(validateFileNameExtension('image.Png', file)).toBeUndefined();
126+
expect(validateFileNameExtension('image.pNG', file)).toBeUndefined();
127+
});
128+
129+
it('should return bad request when extension does not match mime type', () => {
130+
const file = createFile('image/png');
131+
const result = validateFileNameExtension('document.pdf', file);
132+
133+
expect(result).toBeDefined();
134+
expect((result as IKibanaResponse).status).toBe(400);
135+
expect((result as IKibanaResponse).payload).toEqual({
136+
message: 'File extension does not match file type',
137+
});
138+
});
139+
});
140+
});

0 commit comments

Comments
 (0)