Skip to content

Commit f784b72

Browse files
committed
[Files Service] Additional validation for mime-type allowances (elastic#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]> (cherry picked from commit cf5e970) # Conflicts: # src/platform/plugins/shared/files/server/routes/file_kind/create.ts # src/platform/plugins/shared/files/server/routes/file_kind/helpers.ts # src/platform/plugins/shared/files/server/routes/integration_tests/routes.test.ts # src/platform/plugins/shared/files/server/routes/public_facing/download.ts # src/platform/plugins/shared/files/tsconfig.json
1 parent b792fa7 commit f784b72

File tree

12 files changed

+758
-74
lines changed

12 files changed

+758
-74
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: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ import type { FileJSON, FileKind } from '../../../common/types';
1313
import { CreateRouteDefinition, FILES_API_ROUTES } from '../api_routes';
1414
import type { FileKindRouter } from './types';
1515
import * as commonSchemas from '../common_schemas';
16-
import { CreateHandler } from './types';
16+
import { validateMimeType } from './helpers';
17+
import type { CreateHandler } from './types';
1718

1819
export const method = 'post' as const;
1920

@@ -32,25 +33,33 @@ export type Endpoint<M = unknown> = CreateRouteDefinition<
3233
FilesClient['create']
3334
>;
3435

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

5564
export function register(fileKindRouter: FileKindRouter, fileKind: FileKind) {
5665
if (fileKind.http.create) {
@@ -66,7 +75,7 @@ export function register(fileKindRouter: FileKindRouter, fileKind: FileKind) {
6675
},
6776
},
6877
},
69-
handler
78+
createHandler(fileKind)
7079
);
7180
}
7281
}

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 { CreateRouteDefinition, FILES_API_ROUTES } from '../api_routes';
2020

@@ -36,14 +36,23 @@ export const handler: CreateHandler<Endpoint> = async ({ files, fileKind }, req,
3636
const {
3737
params: { id, fileName },
3838
} = req;
39+
3940
const { error, result: file } = await getById(fileService.asCurrentUser(), id, fileKind);
4041
if (error) return error;
42+
4143
try {
44+
const invalidExtensionResponse = validateFileNameExtension(fileName, file);
45+
if (invalidExtensionResponse) {
46+
return invalidExtensionResponse;
47+
}
48+
4249
const body: Response = await file.downloadContent();
50+
const fileHttpResponseOptions = getFileHttpResponseOptions(file);
51+
4352
return res.file({
4453
body,
4554
filename: fileName ?? getDownloadedFileName(file),
46-
headers: getDownloadHeadersForFile({ file, fileName }),
55+
...fileHttpResponseOptions,
4756
});
4857
} catch (e) {
4958
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)