Uploaded image for project: '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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            davosmith 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
            davosmith 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
            davosmith 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
            davosmith 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
            poltawski 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
            poltawski 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
            davosmith 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
            davosmith 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
            davosmith 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
            davosmith 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
            samhemelryk 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
            samhemelryk 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
            davosmith 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
            davosmith 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
            poltawski Dan Poltawski added a comment -

            (submitted for integration for you)

            Show
            poltawski Dan Poltawski added a comment - (submitted for integration for you)
            Hide
            samhemelryk 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
            samhemelryk 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
            stronk7 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
            stronk7 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
            davosmith Davo Smith added a comment -

            Rebased as requested

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

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

            Show
            davosmith Davo Smith added a comment - I've backported the changes onto the 2.0, 2.1 & 2.2 stable branches.
            Hide
            stronk7 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
            stronk7 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
            rajeshtaneja 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
            rajeshtaneja 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
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  12/Mar/12