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

[HttpFoundation] Fix getMaxFilesize #32790

Merged
merged 1 commit into from Jul 30, 2019
Merged

[HttpFoundation] Fix getMaxFilesize #32790

merged 1 commit into from Jul 30, 2019

Conversation

bennyborn
Copy link
Contributor

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 -

@@ -214,7 +214,8 @@ public function move($directory, $name = null)
*/
public static function getMaxFilesize()
{
$iniMax = strtolower(ini_get('upload_max_filesize'));
$iniMax = min([ini_get('upload_max_filesize'), ini_get('post_max_size')]);
Copy link
Member

Choose a reason for hiding this comment

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

min() cannot work here, we should parse post_max_size before calling it, as it can be a string with units (e.g. 8M)
please also add test cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your're right. Would it be okay to add an parseFilesize to the class? Also since there seems to be no test for getMaxFilesize till now I'm not entirely sure how to create one since it depends on the ini settings.

Copy link
Member

Choose a reason for hiding this comment

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

Would it be okay to add an parseFilesize to the class

a private one yes,

not entirely sure how to create one since it depends on the ini settings

dunno if it's possible to use ini_set here... if not, I don't know either :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I've implemented the proper conversion and check on the two values but regarding the tests it seems we're out of luck. We can't set e.g. upload_max_filesize using ini_set() because upload_max_filesize is a PHP_INI_PERDIR type which means it's only changeable via php.ini, .htaccess or httpd.conf ( see php.net/manual/en/configuration.changes.modes.php)

That also explains why there was no test on this method to begin with.

@xabbuh
Copy link
Member

xabbuh commented Jul 30, 2019

Can we add a test case for this?

@nicolas-grekas
Copy link
Member

Not sure, see #32790 (comment)

@nicolas-grekas
Copy link
Member

Thank you @bennyborn.

@nicolas-grekas nicolas-grekas merged commit 54107ba into symfony:3.4 Jul 30, 2019
nicolas-grekas added a commit that referenced this pull request 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`
@bennyborn
Copy link
Contributor Author

Thank you @bennyborn.

No worries, glad I could help!

fabpot added a commit that referenced this pull request Aug 14, 2019
This PR was merged into the 3.4 branch.

Discussion
----------

Fix getMaxFilesize() returning zero

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

With #32790 a BC break got introduced. Previously an empty `upload_max_filesize` returned `PHP_INT_MAX` but after the changes from #32790 it returns `0`.

Setting `upload_max_filesize` or `post_max_size` to `0` or `''` disables the limit so for both cases `PHP_INT_MAX` should be returned.

Commits
-------

f4c2ea5 Fix getMaxFilesize() returning zero
This was referenced Aug 26, 2019
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.

None yet

7 participants