Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[Validator] File: add option to check extension #47710

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.

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

Closed
wants to merge 0 commits into from

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Sep 27, 2022

Q A
Branch? 6.2
Bug fix? no
New feature? no
Deprecations? no
Tickets n/a
License MIT
Doc PR symfony/symfony-docs#17414

This patch adds an extensions option to the File constraint as an alternative to mimeTypes which checks the mime type of the file, its extension, and the consistency between them.

I have a use case where I want to assert that:

  1. the file is of a given mime type
  2. the file has an extension, the extension is in the allow list, and the extension corresponds with the actual mime type of the content

I added a new extension option to the File constraint to do so.

Usage:

#[File(extensions: 'jpg')] // image.jpg is allowed, image.jpeg isn't, allowed mime types are autodetected, the content of the file is automatically checked
#[File(extensions: ['xml' => ['text/xml', 'application/xml'], 'txt'])] // XML files are allowed as long as the extension is .XML, .txt files are allowed if their mime type is text (allowed mime type are auto-detected)

@alanpoulain
Copy link
Contributor

Related 😄 #39063

@dunglas
Copy link
Member Author

dunglas commented Sep 28, 2022

My patch is a bit more advanced than #39063 and prevents real security issues.
Unlike #39063, when you use this new option the validator checks:

  • the mime type of the file content
  • the extension
  • if both are consistent

The other PR was only checking the extension, without checking the actual content of the file. My patch can be seen as an easier way to configure the accepted MIME types and an extra check for consistency between the mime type and the extension.

When using only mime-type validation, you may have issues:

  • You allow text/xml
  • The user uploads a .wsf file (which is technically also an XML document)
  • Another user using Windows downloads the .wsf file, which can be executed

With my patch, if you only allow .xml, a .wsf file will be rejected even if it is valid XML (and the validator will still check that the content of the file is valid XML, as with the old mime-type validation).

@dunglas
Copy link
Member Author

dunglas commented Sep 28, 2022

My patch also prevents uploading files with misleading extensions (.e.g., a GIF named foo.exe), which is currently hard to achieve with Symfony (see the tests).

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor things. Thanks.

src/Symfony/Component/Validator/Constraints/File.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/File.php Outdated Show resolved Hide resolved
@fabpot
Copy link
Member

fabpot commented Oct 1, 2022

Thank you @dunglas.

fabpot added a commit that referenced this pull request Oct 1, 2022
This PR was squashed before being merged into the 6.2 branch.

Discussion
----------

[Validator] File: add option to check extension

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | n/a
| License       | MIT
| Doc PR        | todo

This patch adds an `extensions` option to the `File` constraint as an alternative to `mimeTypes` which checks the mime type of the file, its extension, and the consistency between them.

I have a use case where I want to assert that:

1. the file is of a given mime type
2. the file has an extension, the extension is in the allow list, and the extension corresponds with the actual mime type of the content

I added a new `extension` option to the `File` constraint to do so.

Usage:

```php
#[File(extensions: 'jpg')] // image.jpg is allowed, image.jpeg isn't, allowed mime types are autodetected, the content of the file is automatically checked
#[File(extensions: ['xml' => ['text/xml', 'application/xml'], 'txt'])] // XML files are allowed as long as the extension is .XML, .txt files are allowed if their mime type is text (allowed mime type are auto-detected)
```

Commits
-------

1613e55 [Validator] File: add option to check extension
@fabpot fabpot closed this Oct 1, 2022
@fabpot fabpot force-pushed the feat/validator/extension branch from 93b8e11 to 1613e55 Compare October 1, 2022 13:26
@dunglas dunglas deleted the feat/validator/extension branch October 6, 2022 14:13
fabpot added a commit that referenced this pull request Oct 20, 2022
… (dunglas)

This PR was squashed before being merged into the 6.2 branch.

Discussion
----------

Validator: fix FileValidator when value is an UploadedFile

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | n/a| License       | MIT
| Doc PR        | n/a

The code I introduced in #47710 is currently broken if the file to validate is a `UploadedFile` instance, as we must check the original extension of the file submitted by the client, not the one (that doesn't exist) of the temporary file created on the server by PHP.

This patch fixes the issue.

Commits
-------

e24ef9d Validator: fix FileValidator when value is an UploadedFile
@fabpot fabpot mentioned this pull request Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.