[Files Service] Additional validation for mime-type allowances #234828

tsullivan · 2025-09-11T21:40:57Z

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, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests 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~~
[ ] 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 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~~
Review the backport guidelines 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
...

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

vadimkibana

Nice work! LGTM

vadimkibana · 2025-09-16T08:01:49Z

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

  try {
    const file = await fileService.asInternalUser().getByToken(token);
+
+    const invalidExtensionResponse = validateFileNameExtension(fileName, file);


Nice touch to validate possible extensions on download.

vadimkibana · 2025-09-16T08:06:46Z

...atform/plugins/shared/files/server/routes/file_kind/integration_tests/file_kind_http.test.ts

+          name: 'test.mp4',
+          mimeType: 'video/mp4',


Not for this PR, but food for thought: shall we validate that name and mimeType extensions match? For example, mp4 == mp4 using (validateFileNameExtension() method), or we treat name as this flexible thing that can be anything.

vadimkibana

Maybe, this line now should be removed:

https://github.com/tsullivan/kibana/blob/3fa9324cba8bdab26388a9bee8e7fedf825d298a/src/platform/plugins/shared/files/server/routes/file_kind/upload.ts#L105-L106

tsullivan · 2025-09-17T16:47:17Z

@vadimkibana

Maybe, this line now should be removed:

https://github.com/tsullivan/kibana/blob/3fa9324cba8bdab26388a9bee8e7fedf825d298a/src/platform/plugins/shared/files/server/routes/file_kind/upload.ts#L105-L106

It seems pretty common to use the application/octet-stream content type for file downloads in Kibana. For example, here. Since that would be a change that isn't related to the purpose of this PR, I would prefer to leave that alone.

vadimkibana · 2025-09-18T13:38:44Z

@tsullivan I mean the file upload HTTP endpoint. I guess the linked line:

accepts: fileKind.allowedMimeTypes ?? 'application/octet-stream',

was expected to check for file mime type. Maybe @jloleysens can chime in here.

If so, we should probably remove that line, as it looks to be doing nothing.

Dosant

Lgtm

Discussed offline that maybe we'd need to improve further and actually check the first bytes of the file . Something to potentially follow up on

tsullivan · 2025-09-18T21:15:07Z

x-pack/platform/test/cases_api_integration/common/plugins/cases/server/files/index.ts

    id: CASES_TEST_FIXTURE_FILE_KIND_ID,
    http: fileKindHttpTags(),
-    allowedMimeTypes: ['image/png'],
+    allowedMimeTypes: ['image/png', 'text/plain'],


Some files were uploading text/plain files when that mimeType wasn't originally allowed by this file kind:

./x-pack/platform/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_delete_file_attachments.ts ./x-pack/platform/test/cases_api_integration/security_and_spaces/tests/trial/internal/bulk_delete_file_attachments.ts

…apping'

elasticmachine · 2025-09-23T19:25:03Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: d1e4245

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #3 / security APIs - Session Lifespan Session Lifespan cleanup should properly clean up session expired because of lifespan when providers override global session config

Metrics [docs]

✅ unchanged

History

💔 Build #342724 failed 92603fe
💔 Build #342144 failed b8f3337
💔 Build #341549 failed 9d432d0
💔 Build #341080 failed 5e5cf5e
💚 Build #340486 succeeded fcb65f8
💛 Build #339598 was flaky 3fa9324

cc @tsullivan

kibanamachine · 2025-09-23T19:27:30Z

Starting backport for target branches: 8.18, 8.19, 9.0, 9.1

https://github.com/elastic/kibana/actions/runs/17956892879

kibanamachine · 2025-09-23T19:34:52Z

💔 All backports failed

Status	Branch	Result
❌	8.18	Backport failed because of merge conflicts
❌	8.19	Backport failed because of merge conflicts
❌	9.0	Backport failed because of merge conflicts
❌	9.1	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 234828

Questions ?

Please refer to the Backport tool documentation

…ic#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

tsullivan · 2025-09-24T00:29:48Z

💚 All backports created successfully

Status	Branch	Result
✅	9.1
✅	9.0
✅	8.19
✅	8.18

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…ic#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

…ic#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]>

kibanamachine · 2025-09-24T19:48:44Z

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.
cc: @tsullivan

kibanamachine · 2025-09-25T19:49:04Z

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.
cc: @tsullivan

…#234828) (#236201) # Backport This will backport the following commits from `main` to `8.19`: - [[Files Service] Additional validation for mime-type allowances (#234828)](#234828)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)

…234828) (#236200) # Backport This will backport the following commits from `main` to `9.0`: - [[Files Service] Additional validation for mime-type allowances (#234828)](#234828)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)

…234828) (#236199) # Backport This will backport the following commits from `main` to `9.1`: - [[Files Service] Additional validation for mime-type allowances (#234828)](#234828)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)

…#234828) (#236204) # Backport This will backport the following commits from `main` to `8.18`: - [[Files Service] Additional validation for mime-type allowances (#234828)](#234828)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)

## 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]>

…ic#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]>

tsullivan added 2 commits September 11, 2025 14:34

Validate mimeType from request body

b63b5fb

Add mime-type validation on download

bbda7be

tsullivan self-assigned this Sep 11, 2025

tsullivan added backport:all-open Backport to all branches that could still receive a release release_note:skip Skip the PR/issue when compiling release notes labels Sep 11, 2025

tsullivan added 5 commits September 11, 2025 15:07

Tests and hardening

581f027

update integration tests

c62d7f1

Prevent mime type manipulation from client-sent filename

3f9cae2

Update tests

187abbd

cleanup

5fd0887

tsullivan commented Sep 13, 2025

View reviewed changes

src/platform/plugins/shared/files/server/routes/common.ts Outdated Show resolved Hide resolved

tsullivan and others added 5 commits September 15, 2025 09:51

Merge branch 'main' into fix/team-2022

eff63bc

simplify getDownloadHeadersForFile

8b94103

Fix cases api integration tests

c86fe0c

Merge branch 'main' into fix/team-2022

52e532c

Minor cleanup

3fa9324

tsullivan marked this pull request as ready for review September 15, 2025 22:42

tsullivan requested review from a team as code owners September 15, 2025 22:42

vadimkibana approved these changes Sep 16, 2025

View reviewed changes

vadimkibana reviewed Sep 16, 2025

View reviewed changes

Merge branch 'main' into fix/team-2022

fcb65f8

Dosant approved these changes Sep 18, 2025

View reviewed changes

tsullivan added 2 commits September 18, 2025 10:39

Merge branch 'main' into fix/team-2022

cc3311b

Fix the method of setting res.file with explicit content-type

e581a91

tsullivan commented Sep 18, 2025

View reviewed changes

[CI] Auto-commit changed files from 'node scripts/yarn_deduplicate'

5e5cf5e

janmonschke approved these changes Sep 19, 2025

View reviewed changes

[CI] Auto-commit changed files from 'node scripts/styled_components_m…

d1e4245

…apping'

tsullivan merged commit cf5e970 into elastic:main Sep 23, 2025
13 checks passed

kibanamachine added the v9.2.0 label Sep 23, 2025

tsullivan mentioned this pull request Sep 23, 2025

[9.1] [Files Service] Additional validation for mime-type allowances (#234828) #236199

Merged

tsullivan mentioned this pull request Sep 23, 2025

[9.0] [Files Service] Additional validation for mime-type allowances (#234828) #236200

Merged

tsullivan mentioned this pull request Sep 23, 2025

[8.19] [Files Service] Additional validation for mime-type allowances (#234828) #236201

Merged

tsullivan mentioned this pull request Sep 24, 2025

[8.18] [Files Service] Additional validation for mime-type allowances (#234828) #236204

Merged

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 24, 2025

kibanamachine added the v8.19.5 label Sep 25, 2025

kibanamachine added the v9.0.8 label Sep 25, 2025

kibanamachine added the v9.1.5 label Sep 25, 2025

kibanamachine added v8.18.8 and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Sep 25, 2025

[Files Service] Additional validation for mime-type allowances #234828

[Files Service] Additional validation for mime-type allowances #234828

Uh oh!

Conversation

tsullivan commented Sep 11, 2025 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Identify risks

Uh oh!

Uh oh!

vadimkibana left a comment

Choose a reason for hiding this comment

Uh oh!

vadimkibana Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

vadimkibana Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

vadimkibana left a comment

Choose a reason for hiding this comment

Uh oh!

tsullivan commented Sep 17, 2025

Uh oh!

vadimkibana commented Sep 18, 2025

Uh oh!

Dosant left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 23, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

Uh oh!

Uh oh!

kibanamachine commented Sep 23, 2025

Uh oh!

kibanamachine commented Sep 23, 2025

💔 All backports failed

Manual backport

Questions ?

Uh oh!

tsullivan commented Sep 24, 2025

💚 All backports created successfully

Questions ?

Uh oh!

kibanamachine commented Sep 24, 2025

Uh oh!

kibanamachine commented Sep 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tsullivan commented Sep 11, 2025 •

edited by kibanamachine

Loading

Dosant left a comment •

edited

Loading