Moodle
  1. Moodle
  2. MDL-26890

Forum file size limit is not used if a file is added from privat area.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Files API, Forum
    • Labels:
    • Testing Instructions:
      Hide

      1) Login as admin>goto my profile> private files
      2) Upload atleast two files, one less than 100 kb in size and another more than 100kb in size.
      3) Goto any forum click on edit settings under forum administration
      4) Change max attachment size to 100 kb and save.
      5) try to create a new topic and/or reply to existing topic
      6) try to attach files to the post from your private area
      7) You should be able to attach the file that has a size <100kb easily and should get an error for the other one.
      8) In the later case make sure the file is not present in the post after you close the file picker and make the post.

      Show
      1) Login as admin>goto my profile> private files 2) Upload atleast two files, one less than 100 kb in size and another more than 100kb in size. 3) Goto any forum click on edit settings under forum administration 4) Change max attachment size to 100 kb and save. 5) try to create a new topic and/or reply to existing topic 6) try to attach files to the post from your private area 7) You should be able to attach the file that has a size <100kb easily and should get an error for the other one. 8) In the later case make sure the file is not present in the post after you close the file picker and make the post.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      MDL-26890-master
    • Rank:
      16517

      Description

      moodle .org Using Moodle course forum file upload is limited to 100 KB.So I uploaded a file (140 KB) to my private folder, and added this file from private folder as forum posting attachment. File picker showed to me that the file is to large for the forum and didn't show the file as attachment in edit mode of forum posting. After sending the post to the forum the file was shown as attachment in the forum.

      We've two options to handle this:

      • accept that bigger files can be used by uploading from private area, In this case the error message should be deleted..
      • but normally a teacher will define the max size for file . Then the attachment shouldn't be added to the posting.
      1. 0001-patch-for-MDL-26890.patch
        3 kB
        Ankit Agarwal
      2. patch for MDL--26890.patch
        3 kB
        Ankit Agarwal

        Issue Links

          Activity

          Hide
          Ankit Agarwal added a comment -

          suggested patch for the issue

          Show
          Ankit Agarwal added a comment - suggested patch for the issue
          Hide
          Dongsheng Cai added a comment -

          Hi, Ankit

          Thanks for the patch, problems in your patch:
          1. code indent problem, we use four spaces (not tab) in moodle code, and use unix file format all the time (see more: http://docs.moodle.org/en/Development:Coding_style)

          2. The patch is not a appropriate fix, it is basically copying the code from local/lib.php, the problem is actaully copy_to_area is called before filesize limit check.

          Show
          Dongsheng Cai added a comment - Hi, Ankit Thanks for the patch, problems in your patch: 1. code indent problem, we use four spaces (not tab) in moodle code, and use unix file format all the time (see more: http://docs.moodle.org/en/Development:Coding_style ) 2. The patch is not a appropriate fix, it is basically copying the code from local/lib.php, the problem is actaully copy_to_area is called before filesize limit check.
          Hide
          Ankit Agarwal added a comment -

          Hi Dongsheng,

          Thanks for the comments.

          I figured out that we need to check the filesize before we make a call to copy_to_area function.But I didnt get any pre-defined function of directly checking the filesize of the local files.Did i missed something in the docs?

          On the other note..i am trying to get familiar with the moodle coding style..will keep your suggestion in mind.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Dongsheng, Thanks for the comments. I figured out that we need to check the filesize before we make a call to copy_to_area function.But I didnt get any pre-defined function of directly checking the filesize of the local files.Did i missed something in the docs? On the other note..i am trying to get familiar with the moodle coding style..will keep your suggestion in mind. Thanks
          Hide
          Ankit Agarwal added a comment -

          As suggested by Dongsheng ,i have used a method instead of hard coding the process.
          attached patch for the same.
          Thanks

          Show
          Ankit Agarwal added a comment - As suggested by Dongsheng ,i have used a method instead of hard coding the process. attached patch for the same. Thanks
          Hide
          Dongsheng Cai added a comment -

          Thanks for the patch, it looks good to me, raised the priority of this issue, added to stable backlog.

          Show
          Dongsheng Cai added a comment - Thanks for the patch, it looks good to me, raised the priority of this issue, added to stable backlog.
          Hide
          Sam Hemelryk added a comment -

          Hi Ankit looks good to me.
          Only thing to do before submitting for integration is clean up the indenting of `return $filesize` lib.php line 1381.
          Other than that spot on.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Ankit looks good to me. Only thing to do before submitting for integration is clean up the indenting of `return $filesize` lib.php line 1381. Other than that spot on. Cheers Sam
          Hide
          Ankit Agarwal added a comment -

          Thanks Sam for the review

          Show
          Ankit Agarwal added a comment - Thanks Sam for the review
          Hide
          Petr Škoda added a comment -

          oh!
          $filearea = clean_param($params['filearea'], PARAM_ALPHAEXT);
          $component = clean_param($params['component'], PARAM_ALPHAEXT);

          is very wrong, it is already in repository/lib.php:
          1/ unfortuantely we do not have PARAM_FRANKENSTYLE yet for component
          2/ PARAM_SAFEDIR might be best for the filearea

          I have create a new issue for that MDL-29401

          Show
          Petr Škoda added a comment - oh! $filearea = clean_param($params ['filearea'] , PARAM_ALPHAEXT); $component = clean_param($params ['component'] , PARAM_ALPHAEXT); is very wrong, it is already in repository/lib.php: 1/ unfortuantely we do not have PARAM_FRANKENSTYLE yet for component 2/ PARAM_SAFEDIR might be best for the filearea I have create a new issue for that MDL-29401
          Hide
          Aparup Banerjee added a comment -

          just linking Petr's issue.

          Show
          Aparup Banerjee added a comment - just linking Petr's issue.
          Hide
          Aparup Banerjee added a comment - - edited

          Hi Ankit,
          I've just had a look at your patch and I have some notes:

          • there seems to be a consistent space after calls to the empty function, perhaps this could be your editor setting ?
          • as Petr said, PARAM_SAFEDIR does seem appropriate for filearea. (checkout clean_param() function)

          ps: you may want to watch MDL-29401 in case he's already got a patch there to validate component names.
          cheers,
          Aparup

          Show
          Aparup Banerjee added a comment - - edited Hi Ankit, I've just had a look at your patch and I have some notes: there seems to be a consistent space after calls to the empty function, perhaps this could be your editor setting ? as Petr said, PARAM_SAFEDIR does seem appropriate for filearea. (checkout clean_param() function) ps: you may want to watch MDL-29401 in case he's already got a patch there to validate component names. cheers, Aparup
          Hide
          Aparup Banerjee added a comment -

          Thanks for the fixes , they've now been integrated.

          Show
          Aparup Banerjee added a comment - Thanks for the fixes , they've now been integrated.
          Hide
          Michael de Raadt added a comment -

          Test result: passed. Well done!

          Show
          Michael de Raadt added a comment - Test result: passed. Well done!
          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:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: