Moodle
  1. Moodle
  2. MDL-21300

Unable to upload of user profile pictures in bulk when username contains special characters

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.7
    • Fix Version/s: 1.9.8, 2.0
    • Component/s: Administration
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      26545

      Description

      if username contains special characters (extendedusernamechars = true), user picture will not be uploaded.

        Activity

        Hide
        Rossiani Wijaya added a comment -

        removing the validation for clean_param for filename.

        Sam what's your opinion with the solution?

        Rosie

        Show
        Rossiani Wijaya added a comment - removing the validation for clean_param for filename. Sam what's your opinion with the solution? Rosie
        Hide
        Sam Hemelryk added a comment -

        Hi Rossi,
        By the looks of it the clean_param is required to ensure that the files contained in the uploaded zip files don't have malicious file names.
        I certainly think we need to clean/check them, perhaps it can be done in the same way that we process usernames when uploading users, was there a PARAM_ that you used for that?
        I've added Petr as watcher:
        Petr, perhaps you could advise us about the security issues of using a more lenient check?

        Show
        Sam Hemelryk added a comment - Hi Rossi, By the looks of it the clean_param is required to ensure that the files contained in the uploaded zip files don't have malicious file names. I certainly think we need to clean/check them, perhaps it can be done in the same way that we process usernames when uploading users, was there a PARAM_ that you used for that? I've added Petr as watcher: Petr, perhaps you could advise us about the security issues of using a more lenient check?
        Hide
        Rossiani Wijaya added a comment -

        Thanks for the review Sam.

        The reason, i removed it because with 'User attribute to use to match pictures: username' the file name is used to match with the username in DB. if a user was created with extendedusernamechars enabled, then username may contain special characters such as #$~. if we use validation on the file name to eliminate malicious character, the system will skip uploading profile picture for the user.

        I did create PARAM_USERNAME to validate username, however it relies on the current setting for extenedusernamechars. a user maybe created with the extendedchars enable and disable it right after. in this case, uploading profile picture will be skip for that user.

        Note for petr: PARAM_USERNAME has not been committed yet. Please refer to MDL-16919 for reference.

        Show
        Rossiani Wijaya added a comment - Thanks for the review Sam. The reason, i removed it because with 'User attribute to use to match pictures: username' the file name is used to match with the username in DB. if a user was created with extendedusernamechars enabled, then username may contain special characters such as #$~. if we use validation on the file name to eliminate malicious character, the system will skip uploading profile picture for the user. I did create PARAM_USERNAME to validate username, however it relies on the current setting for extenedusernamechars. a user maybe created with the extendedchars enable and disable it right after. in this case, uploading profile picture will be skip for that user. Note for petr: PARAM_USERNAME has not been committed yet. Please refer to MDL-16919 for reference.
        Hide
        Petr Škoda added a comment -

        The file names are cleaned during unzipping, I doubt there is any way to compensate for loss of this information later, so the current code can not detect it. The patch makes sense to me, and the problematic pictures have to end with warning notify(get_string('uploadpicture_usernotfound', 'admin', $a));

        I personally think that people should not use funny characters in usernames, and if they do they should setup some meaningful user IDnumbers and use them instead of usernames when uploading pictures.

        So +1 for commit

        Show
        Petr Škoda added a comment - The file names are cleaned during unzipping, I doubt there is any way to compensate for loss of this information later, so the current code can not detect it. The patch makes sense to me, and the problematic pictures have to end with warning notify(get_string('uploadpicture_usernotfound', 'admin', $a)); I personally think that people should not use funny characters in usernames, and if they do they should setup some meaningful user IDnumbers and use them instead of usernames when uploading pictures. So +1 for commit
        Hide
        Rossiani Wijaya added a comment -

        Petr, Thank you for the input.

        committed patch to 19_stable and HEAD.

        Show
        Rossiani Wijaya added a comment - Petr, Thank you for the input. committed patch to 19_stable and HEAD.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: