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.

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

File manager wrong way to determine max upload size #498

Closed
bennyborn opened this issue May 24, 2019 · 8 comments
Closed

File manager wrong way to determine max upload size #498

bennyborn opened this issue May 24, 2019 · 8 comments

Comments

@bennyborn
Copy link
Contributor

Affected version(s)
At least from 4.4.39 upwards

Description
When trying to upload a file the file manager determines the maximum allowed file size mainly by looking at the the value set in the localconfig. In fact there are 4 values that need to be checked to determine the real limit:

  • php.ini memory_limit
  • php.ini post_max_size
  • php.ini upload_max_filesize
  • Setting from localconfig

If we don't check the php.ini we`ll get some nice side effects.

I was trying to upload a file (>10 MB) in an installation that has a 20MB limit set in the localconfig as well as upload_max_filesize. While the memory_limit was plenty enough no one has thought of setting post_max_size accordingly.

When I now try to upload a file thats larger than post_max_size the DropZone uploader will add a copy of the file manger to itself. Needless to say that the upload fails without any notification.

See the screenshot attached

image

How to reproduce

  1. Set the maximum filesize in your localconfig to 10MB.
  2. Make sure at least one of the PHP ini settings from above is below 10MB.
  3. Try to upload a 10MB file.
@leofeyer leofeyer transferred this issue from contao/core-bundle May 24, 2019
@aschempp
Copy link
Member

I don't think memory_limit is related to uploads?

@bennyborn
Copy link
Contributor Author

Not to the upload itself of course but the handling afterwards. TBH I haven't looked into this so I'm not sure if the core does anything other than just copying the file to it's new destination.

@leofeyer
Copy link
Member

We are using the Symfony file size check in combination with the localconfig setting:

public static function getMaxUploadSize()
{
return min(UploadedFile::getMaxFilesize(), Config::get('maxFileSize'));
}

And the Symfony check is based on upload_max_filesize:

https://github.com/symfony/symfony/blob/2ab152138870b9eead8bed01606605da043f1a14/src/Symfony/Component/HttpFoundation/File/UploadedFile.php#L246

So if you think that post_max_size should be considered (I second this) or that memory_limit should be considered (I do not second this), the best thing to do is to create a PR for the Symfony UploadedFile class.

@aschempp
Copy link
Member

I'm assuming this issue should be reopened then until the Symfony issue is fixed? Who'll create the Symfony PR?

@aschempp aschempp reopened this Jul 28, 2019
@bennyborn
Copy link
Contributor Author

bennyborn commented Jul 28, 2019

[...] Who'll create the Symfony PR?

Looking into it...

@leofeyer
Copy link
Member

I'm assuming this issue should be reopened then until the Symfony issue is fixed?

Why? There is nothing to do on our side.

@aschempp
Copy link
Member

the best thing to do is to create a PR for the Symfony UploadedFile class.

I thought this is? And have a reference about this issue in Contao until it's fixed in Symfony?

nicolas-grekas added a commit to symfony/symfony that referenced this issue Jul 30, 2019
This PR was squashed before being merged into the 3.4 branch (closes #32790).

Discussion
----------

[HttpFoundation] Fix `getMaxFilesize`

When checking for the maximum size of an uploaded file you can't just rely on `upload_max_filesize` since the request might also exceed `post_max_size`. Also discussed in contao/contao#498

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Commits
-------

54107ba [HttpFoundation] Fix `getMaxFilesize`
@leofeyer
Copy link
Member

symfony/symfony#32790 has been merged. Thank you @bennyborn.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants