[media] Special characters issue #7386

pierre-p-s · 2021-03-11T17:39:15Z

Brief summary of changes

Solves an issue when if using special characters (&, <, >, ") the file download fails and the name of the file appears incorrectly in the browse tab of the media module.
The script removes the special characters from the sql data table media as well as from the file system.

Testing instructions (if applicable)

Before checking out this branch, on 23.0-release, upload a file with special characters &, <, >, " into media.
Check that the file name appears incorrectly in the media table
Check out this branch.
Upload a different file with special characters &, <, >, " into media.
Look for the file in the browse tab, check that the file name appears correctly
Try downloading the file and opening it.
run (need sudo to be able to change the name of the files under /data/media_uploads/): sudo php /var/www/loris/tools/single_use/Cleanup_Special_Chars_Media.php
Check that the first file now appears correctly in the data table and that when downloading you are able to read the file correctly.

pierre-p-s · 2021-03-11T18:55:06Z

The test suite seems to be failing when running make checkstatic however on my VM the command runs without errors.

pierre-p-s · 2021-03-15T17:52:12Z

Blocked by #7390

pierre-p-s · 2021-03-18T14:59:59Z

After discussion with Rida: Remove encoding when inserting into SQL table, line 186 of fileUpload use unsafe insert and fix issues in the database

pierre-p-s · 2021-03-19T16:20:41Z

@ridz1208 Let me know if this is a better fix please.

ridz1208 · 2021-03-19T17:02:07Z

modules/media/ajax/FileUpload.php

@@ -130,6 +130,7 @@ function uploadFile()
    checkDateTaken($dateTaken);

    $fileName  = preg_replace('/\s/', '_', $_FILES["file"]["name"]);
+    $fileName  = str_replace("%22", "\"", $fileName);


does this mean you are accepting quotes in the name of the file ? also its a bit clearer if you use '"' in the second argument

Well I wanted to include the double quotes because right now we can add a file with it in the name. However we need this check fo it to work. Otherwise I could also send an error through swal refusing the quotes, but in that case what else are we not allowing?

%22 is URI encoding, your $filename is never coming from the URI... I'm not clear on why its being "decoded" (and BTW decoding should be done with urldecode() https://www.php.net/manual/en/function.urldecode.php not manually like this)

I changed it to urldecode but I cant find where the fileName is being encoded...

pierre-p-s · 2021-03-24T00:48:30Z

This PR still needs a script to change the file names in the sql table as well as in /data/uploads

modules/media/ajax/FileUpload.php

Co-authored-by: Rida Abou-Haidar <ridz1208@users.noreply.github.com>

tools/single_use/Cleanup_Special_Chars_Media.php

jesscall · 2021-04-12T16:18:40Z

Looks good!

driusan · 2021-04-15T18:12:09Z

tools/single_use/Cleanup_Special_Chars_Media.php

+        );
+
+        // update name in file system
+        shell_exec("mv " . escapeshellarg($media_path . $fileNameURLencoded) . " " . escapeshellarg($media_path . $fileName));


Why isn't this just using rename?

https://www.php.net/manual/en/function.rename.php

I missed it while reviewing the document_repository PR, but can you change it in that script too?

I changed it to rename and I will create a new PR for the document repository script and link it here

changed the doc repo script in this PR: #7428

@driusan let me know if it needs more changes

Fixes script from this PR #7383 See comment from #7386

ridz1208 · 2022-02-05T19:28:13Z

tools/single_use/Cleanup_Special_Chars_Media.php

@@ -0,0 +1,54 @@
+<?php


[media] Special characters issue

e116277

pierre-p-s added the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Mar 11, 2021

kongtiaowang mentioned this pull request Mar 15, 2021

[git actions] Phan can't run in php 7.3 #7390

Closed

pierre-p-s added Blocked PR awaiting the merge, resolution or rejection of an other Pull Request and removed "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help labels Mar 15, 2021

ridz1208 added the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Mar 18, 2021

Addressing Rida's comments

14b7a8d

ridz1208 reviewed Mar 19, 2021

View reviewed changes

Addressing Rida's comment

f2277e6

pierre-p-s removed Blocked PR awaiting the merge, resolution or rejection of an other Pull Request Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix labels Mar 23, 2021

ridz1208 reviewed Mar 24, 2021

View reviewed changes

modules/media/ajax/FileUpload.php Show resolved Hide resolved

pierre-p-s added the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Mar 24, 2021

pierre-p-s added 2 commits March 31, 2021 12:47

First pass a script

e3dd1e9

Script working

f307fe6

pierre-p-s removed the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Mar 31, 2021

Update modules/media/ajax/FileUpload.php

53fbc4a

Co-authored-by: Rida Abou-Haidar <ridz1208@users.noreply.github.com>

pierre-p-s requested a review from ridz1208 March 31, 2021 18:13

pierre-p-s added 2 commits March 31, 2021 14:36

Trying to fix test suite

0d80019

Fixed test suite

f94e4df

jesscall suggested changes Mar 31, 2021

View reviewed changes

tools/single_use/Cleanup_Special_Chars_Media.php Outdated Show resolved Hide resolved

tools/single_use/Cleanup_Special_Chars_Media.php Outdated Show resolved Hide resolved

Addressing Jessica's comments

aa2ecd3

pierre-p-s requested a review from jesscall April 1, 2021 01:34

jesscall approved these changes Apr 12, 2021

View reviewed changes

jesscall added the Passed Manual Tests PR has undergone proper testing by at least one peer label Apr 12, 2021

driusan requested changes Apr 15, 2021

View reviewed changes

pierre-p-s added 2 commits April 16, 2021 11:32

Addressing Dave's comment

e372f74

Solves escape issue

1ada058

pierre-p-s mentioned this pull request Apr 16, 2021

[document_repository] Fix special characters script #7428

Merged

driusan pushed a commit that referenced this pull request Apr 20, 2021

[document_repository] Fix special characters script (#7428)

7c195a3

Fixes script from this PR #7383 See comment from #7386

driusan merged commit e7e3650 into aces:23.0-release Apr 20, 2021

ridz1208 added this to the 23.0.3 milestone Apr 20, 2021

ridz1208 reviewed Feb 5, 2022

View reviewed changes

tools/single_use/Cleanup_Special_Chars_Media.php

@@ -0,0 +1,54 @@

<?php

Copy link

Collaborator

ridz1208 Feb 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before SQL

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[media] Special characters issue #7386

[media] Special characters issue #7386

pierre-p-s commented Mar 11, 2021 •

edited

pierre-p-s commented Mar 11, 2021

pierre-p-s commented Mar 15, 2021

pierre-p-s commented Mar 18, 2021

pierre-p-s commented Mar 19, 2021

ridz1208 Mar 19, 2021

pierre-p-s Mar 19, 2021

ridz1208 Mar 19, 2021

pierre-p-s Mar 23, 2021

pierre-p-s commented Mar 24, 2021

jesscall commented Apr 12, 2021

driusan Apr 15, 2021

driusan Apr 15, 2021

pierre-p-s Apr 16, 2021 •

edited

pierre-p-s Apr 16, 2021

pierre-p-s Apr 16, 2021

ridz1208 Feb 5, 2022

[media] Special characters issue #7386

[media] Special characters issue #7386

Conversation

pierre-p-s commented Mar 11, 2021 • edited

Brief summary of changes

Testing instructions (if applicable)

pierre-p-s commented Mar 11, 2021

pierre-p-s commented Mar 15, 2021

pierre-p-s commented Mar 18, 2021

pierre-p-s commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierre-p-s commented Mar 24, 2021

jesscall commented Apr 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierre-p-s Apr 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierre-p-s commented Mar 11, 2021 •

edited

pierre-p-s Apr 16, 2021 •

edited