Moodle
  1. Moodle
  2. MDL-31113

Multiple files can be uploaded by File manager with max file set to x

    Details

    • Testing Instructions:
      Hide

      NOTE to testers: This needs to be tested under STABLE (21 or 22) and master. And master needs to be tested with normal (upload) and also drag and drop one.

      1. Go to course -> forum
      2. upload a file (File will be visible in file manager)
      3. Press browser back button
      4. Press browser forward button
      5. file should be visible in file manager
      6. try upload another file
      7. You should get error message
      Show
      NOTE to testers: This needs to be tested under STABLE (21 or 22) and master. And master needs to be tested with normal (upload) and also drag and drop one. Go to course -> forum upload a file (File will be visible in file manager) Press browser back button Press browser forward button file should be visible in file manager try upload another file You should get error message
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31113_multiple_files_file_manager
    • Rank:
      37542

      Description

      User can fool filemanager to upload multiple files even though max files is set.

      1. Go to course -> forum
      2. upload a file (File will be visible in file manager)
      3. Press browser back button
      4. Press browser forward button
      5. No file is visible in file manager
      6. upload another file
      7. You should see two files uploaded
      8. Save and both files get saved.

      Using Drag and drop (DND) also you can see this issue:

      1. Upload a file using DND
      2. Try uploading another file (I used PDF)
      3. PDF opens (Which user don't expect IMO)
      4. Press browser back button, no file is visible
      5. Drag and drop another file, you will see two file. (This is related to file manager issue mentioned below.)
      6. Save post and both files get uploaded.

        Issue Links

          Activity

          Hide
          Davo Smith added a comment -

          I can reproduce this and there are basically 2 issues.

          1. Dragging & dropping extra files past the limit should still cancel the browser default behaviour of displaying the file (even if no file is uploaded)
          2. Moving back again should not allow you to upload extra files (this may be more tricky to fix, but may not be possible to reproduce after the first bug has been fixed).

          Show
          Davo Smith added a comment - I can reproduce this and there are basically 2 issues. 1. Dragging & dropping extra files past the limit should still cancel the browser default behaviour of displaying the file (even if no file is uploaded) 2. Moving back again should not allow you to upload extra files (this may be more tricky to fix, but may not be possible to reproduce after the first bug has been fixed).
          Hide
          Davo Smith added a comment -

          I can't find any way to assign the issue to myself, but I will consider it as such

          Show
          Davo Smith added a comment - I can't find any way to assign the issue to myself, but I will consider it as such
          Hide
          Dan Poltawski added a comment -

          Davo - I think we need to get you made into a 'jira developer' to assign the issue to you. In the meantime I have assigned to me so nobody else grabs it

          Show
          Dan Poltawski added a comment - Davo - I think we need to get you made into a 'jira developer' to assign the issue to you. In the meantime I have assigned to me so nobody else grabs it
          Hide
          Davo Smith added a comment -

          Looking at this, there appears to be a bug in the filemanager element itself.

          If you upload a file (using the 'Add...' button), then browse to another page, press the back button and upload another file (again with the 'Add...' button), then you end up with multiple files uploaded (even if the limit was set to 1).

          I'll post this as a separate bug, as it isn't directly related to the drag and drop upload (I've fixed the other issue and will attach a patch, once I've prepared it (it just involves editing the 'check_drag' function to move e.preventDefault & e.stopPropagation above the 'maxfiles' check).

          Show
          Davo Smith added a comment - Looking at this, there appears to be a bug in the filemanager element itself. If you upload a file (using the 'Add...' button), then browse to another page, press the back button and upload another file (again with the 'Add...' button), then you end up with multiple files uploaded (even if the limit was set to 1). I'll post this as a separate bug, as it isn't directly related to the drag and drop upload (I've fixed the other issue and will attach a patch, once I've prepared it (it just involves editing the 'check_drag' function to move e.preventDefault & e.stopPropagation above the 'maxfiles' check).
          Hide
          Davo Smith added a comment -

          Here is a patch branch (based on current master):
          https://github.com/davosmith/moodle/tree/MDL-31113_multiple_files_file_manager
          Diff:
          https://github.com/davosmith/moodle/commit/ab3f4b73b41ceb86f82b2f9d33041d2d0659f2eb

          The changes are as follows:

          • drag and drop upload now prevents default browser drop behaviour even when max files reached (prevents browser trying to display files after limit reached)
          • filemanager element refreshes its file list from the server on initialisation
          • filemanager AJAX 'list' call now returns the total file count (as otherwise the count will be 0 even if the initial 'refresh' returns files)
          • filemanager update the add/download button display when 'refresh' call finishes

          Unfortunately, I don't seem to have permission on tracker to set the patch URL or request a review (although even if I could, I'm guessing I'd need some guidance about which buttons to click anyway!).

          Note: this is still not a complete solution, as there is still no server-side validation of maxfiles on form submission (so you can avoid the limit by using Chrome dev tools to show the add button, then upload more files).

          Show
          Davo Smith added a comment - Here is a patch branch (based on current master): https://github.com/davosmith/moodle/tree/MDL-31113_multiple_files_file_manager Diff: https://github.com/davosmith/moodle/commit/ab3f4b73b41ceb86f82b2f9d33041d2d0659f2eb The changes are as follows: drag and drop upload now prevents default browser drop behaviour even when max files reached (prevents browser trying to display files after limit reached) filemanager element refreshes its file list from the server on initialisation filemanager AJAX 'list' call now returns the total file count (as otherwise the count will be 0 even if the initial 'refresh' returns files) filemanager update the add/download button display when 'refresh' call finishes Unfortunately, I don't seem to have permission on tracker to set the patch URL or request a review (although even if I could, I'm guessing I'd need some guidance about which buttons to click anyway!). Note: this is still not a complete solution, as there is still no server-side validation of maxfiles on form submission (so you can avoid the limit by using Chrome dev tools to show the add button, then upload more files).
          Hide
          Sam Hemelryk added a comment -

          Hi Davo,

          Changes you've made here look spot on thanks. Feel free to put it up for integration when you are ready.

          Quick question it appears you've fixed the noted back button issue in your changes here. Did you end up creating an issue for it?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Davo, Changes you've made here look spot on thanks. Feel free to put it up for integration when you are ready. Quick question it appears you've fixed the noted back button issue in your changes here. Did you end up creating an issue for it? Cheers Sam
          Hide
          Davo Smith added a comment -

          I have fixed both problems raised here - the back button issue and the dragging past the file limit opening the file (which is the one which should have a separate issue, of the two of them).

          I haven't actually created a separate issue for either of these - should I do so?

          And I don't appear to have any ability to request integration.

          Show
          Davo Smith added a comment - I have fixed both problems raised here - the back button issue and the dragging past the file limit opening the file (which is the one which should have a separate issue, of the two of them). I haven't actually created a separate issue for either of these - should I do so? And I don't appear to have any ability to request integration.
          Hide
          Dan Poltawski added a comment -

          (submitted for integration for you)

          Show
          Dan Poltawski added a comment - (submitted for integration for you)
          Hide
          Sam Hemelryk added a comment -

          Hi Davo,

          I'm thinking that it would be great to backport the back button issue assuming that it affecting the stable branches.
          Either way we can do that as a separate issue of course and can do it after this has been integrated in fact if we want.

          Dan thanks for putting this up for integration

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Davo, I'm thinking that it would be great to backport the back button issue assuming that it affecting the stable branches. Either way we can do that as a separate issue of course and can do it after this has been integrated in fact if we want. Dan thanks for putting this up for integration Cheers Sam
          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
          Davo Smith added a comment -

          Rebased as requested

          Show
          Davo Smith added a comment - Rebased as requested
          Hide
          Davo Smith added a comment -

          I've backported the changes onto the 2.0, 2.1 & 2.2 stable branches.

          Show
          Davo Smith added a comment - I've backported the changes onto the 2.0, 2.1 & 2.2 stable branches.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (21, 22 and master), thanks!

          Note: Should this be considered a security issue? Feel free to mark it if so.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (21, 22 and master), thanks! Note: Should this be considered a security issue? Feel free to mark it if so.
          Hide
          Rajesh Taneja added a comment -

          Works Great.
          Thanks for the awesome work, Davo
          FYI: Tested on 21, 22 and master

          @Eloy: Not sure if it's a security issue, but I think it will be nice to back-port this to 20 branch as well.

          Show
          Rajesh Taneja added a comment - Works Great. Thanks for the awesome work, Davo FYI: Tested on 21, 22 and master @Eloy: Not sure if it's a security issue, but I think it will be nice to back-port this to 20 branch as well.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Your nice code represents only 1/46 of the issues that have been sent upstream this week, so thanks, but not many.

          Nah, joking, many thanks! Closing this a fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Your nice code represents only 1/46 of the issues that have been sent upstream this week, so thanks, but not many. Nah, joking, many thanks! Closing this a fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: