Moodle
  1. Moodle
  2. MDL-29036

webservice/upload.php should respect max file size

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.2
    • Fix Version/s: 2.1.2
    • Component/s: Web Services
    • Labels:
    • Testing Instructions:
      Hide

      The easiest way to test this is using the moodle iphone app (ask me for it when you test it)

      First, to test the maxbytes setting:
      1. site admin -> Site policies -> maxbytes, set to a small value, for example, 10k
      2. pick a picture from iphone, try upload it, it should fail

      Second, test the userquota value
      1. site admin -> Site policies -> userquota, set to a small value, for example, 10k
      2. pick a picture from iphone, try upload, it should fail

      Alternative workaround for second test through the site:
      1. site admin -> Site policies -> userquota, set to a small value, for example, 10k
      2. on navigation menu, click My profile > My private file
      3. click on "Manage my private files" button.
      4. try to upload a file greater than 10k size. this should fail

      Show
      The easiest way to test this is using the moodle iphone app (ask me for it when you test it) First, to test the maxbytes setting: 1. site admin -> Site policies -> maxbytes, set to a small value, for example, 10k 2. pick a picture from iphone, try upload it, it should fail Second, test the userquota value 1. site admin -> Site policies -> userquota, set to a small value, for example, 10k 2. pick a picture from iphone, try upload, it should fail Alternative workaround for second test through the site: 1. site admin -> Site policies -> userquota, set to a small value, for example, 10k 2. on navigation menu, click My profile > My private file 3. click on "Manage my private files" button. 4. try to upload a file greater than 10k size. this should fail
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull Master Branch:
      s13_MDL-29036_ws_upload_master
    • Rank:
      18568

      Description

      Reported by Gavin on MOBILE-108.

      reset the limit to 500k on server (Maximum uploaded file size) on the site policies

      This was Confirmed as working on UI of upload private files in webpage "Maximum size for new files: 100KB")

      Connected to site on IPhone 3GS using Wireless

      Took a video (was 10 seconds) and pressed "Use"

      It submitted and said completed.

      I confirmed that it did appear in my private files.

      When downloading the video it was 1.2 Mb which exceeds the upload limits.

      I reset the limit to 100k on server (Maximum uploaded file size) on the site policies

      Uploaded an image successfully too, which had 700kb in size.

      Is the upload ignoring the file max size?

      Is it web service or app issue? or both?

      This was on a Moodle 2.1.1 build.

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Hi DS,

          I've had a look at this and it looks good, just a couple of things I noticed may or may not need attention.

          1. In checking the size of each file you call filesize on the tmp_name, would it be easier just to use the size property that is provided for uploaded files? or is the a particular reason this has been avoided?
          1. If the total user quota would be exceeded the script throws an exception however for other errors it adds a string to the $results array. Should this exception be changed to an error string and returned in an array to simplify things for the client?

          Other than those two areas to clarify it looks good to me.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi DS, I've had a look at this and it looks good, just a couple of things I noticed may or may not need attention. In checking the size of each file you call filesize on the tmp_name, would it be easier just to use the size property that is provided for uploaded files? or is the a particular reason this has been avoided? If the total user quota would be exceeded the script throws an exception however for other errors it adds a string to the $results array. Should this exception be changed to an error string and returned in an array to simplify things for the client? Other than those two areas to clarify it looks good to me. Cheers Sam
          Hide
          Dongsheng Cai added a comment -

          Thanks sam

          1. I am going to change to use `size` property
          2. the maxbytes limit is applied to each file, I don't think we should throw exception if one oversized file. For userquota, it's the overall size, it's hard to tell which file make it over sized, so I think it make more sense to reject this upload.

          Regards,
          Dongsheng

          Show
          Dongsheng Cai added a comment - Thanks sam I am going to change to use `size` property the maxbytes limit is applied to each file, I don't think we should throw exception if one oversized file. For userquota, it's the overall size, it's hard to tell which file make it over sized, so I think it make more sense to reject this upload. Regards, Dongsheng
          Hide
          Sam Hemelryk added a comment -

          Cool thanks DS, sounds good to me

          Show
          Sam Hemelryk added a comment - Cool thanks DS, sounds good to me
          Hide
          Dongsheng Cai added a comment -

          Rebased, and changed to _FILE['element']['size'] instead of calculating file size using php function

          Show
          Dongsheng Cai added a comment - Rebased, and changed to _FILE ['element'] ['size'] instead of calculating file size using php function
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Aparup Banerjee added a comment -

          This has been integrated. cheers!

          Show
          Aparup Banerjee added a comment - This has been integrated. cheers!
          Hide
          Rossiani Wijaya added a comment -

          This works great

          Test passed.

          Show
          Rossiani Wijaya added a comment - This works great Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing. Ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: