znuny / Znuny

Znuny/Znuny LTS is a fork of the ((OTRS)) Community Edition, one of the most flexible web-based ticketing systems used for Customer Service, Help Desk, IT Service Management.
https://www.znuny.org
GNU General Public License v3.0
335 stars 82 forks source link

Upload cache fixes #439

Open pboguslawski opened 1 year ago

pboguslawski commented 1 year ago

Proposed change

This mod fixes the following upload cache problems:

(1) If one uploads many files (dropping files test0.bin-test9.bin in one move) races may occur and file id in DOM won't match file ids in web upload dir on server, i.e.:

$(".AttachmentListContainer tbody tr").filter(function() { console.log("%s, %d", $(this).find('td.Filename').text(), $(this).find('a').data('file-id'))})
test9.bin, 10
test8.bin, 8
test7.bin, 8
test6.bin, 7
test5.bin, 4
test4.bin, 5
test3.bin, 3
test2.bin, 3
test1.bin, 2
test0.bin, 1

This may be fixed with patch (which unblocks updating all file ids on upload not just for one file)...

--- a/var/httpd/htdocs/js/Core.UI.js   2021-11-08 18:53:34.088284466 +0100
+++ b/var/httpd/htdocs/js/Core.UI.js 2021-12-16 18:41:14.090199778 +0100
@@ -697,10 +697,6 @@

                                 $TargetObj = $ExistingItemObj.closest('tr');

-                                if ($TargetObj.find('a').data('file-id')) {
-                                    return;
-                                }
-
                                 $TargetObj
                                     .find('.Filetype')
                                     .text(Attachment.ContentType)

...but using file ids for identification of uploaded files is not race condition resistant; to provoke race condition apply patch

--- a/Kernel/Modules/AjaxAttachment.pm 2021-06-19 21:59:34.379471088 +0200
+++ b/Kernel/Modules/AjaxAttachment.pm       2021-12-16 18:38:28.943925139 +0100
@@ -56,6 +56,10 @@
             Param => 'Files',
         );

+        if ($UploadStuff{Filename} eq 'test01.bin') {
+            sleep(1);
+        }
+
         $UploadCacheObject->FormIDAddFile(
             FormID      => $Self->{FormID},
             Disposition => 'attachment',
@@ -67,6 +71,10 @@
             FormID => $Self->{FormID},
         );

+        if ($UploadStuff{Filename} eq 'test02.bin') {
+            sleep(2);
+        }
+
         my @AttachmentData;

and drop files test01.bin and test02.bin (same size) in one move; after upload there will be:

$(".AttachmentListContainer tbody tr").filter(function() { console.log("%s, %d", $(this).find('td.Filename').text(), $(this).find('a').data('file-id'))})
test02.bin, 1
test01.bin, 1

not

test02.bin, 2
test01.bin, 1

This problem was solved by removing unnecessary FileID attribute from upload data struct to avoid any races that may occur between filename list and fileid list. Only filename will be used to identify items added to upload cache.

This bug is dangerous - think about uploading some confidential stuff by mistake, then removing it (disappears from UI) and sending message with different set of files than displayed in UI (i.e. with confidential stuff that user wanted to remove).

(2) Double clicking trash icon to remove attachment generates two requests to server and one will cause an error.

(3) Unused upload handling code removed.

Type of change

Breaking change

Removes FileID attribute from web upload cache objects to avoid races.

Additional information

Replaces: https://github.com/znuny/Znuny/pull/179 Author-Change-Id: IB#1113065

Checklist