Manually merge PR139 to show a more user-friendly error message in file manager if download hits max bytes limit, and generally improve the error delivery for requests using the explicit download 'file' output_format marker.

jonasbardino · jonasbardino · commit 3766076f1633 · 2024-10-22T13:56:14.000Z
git-svn-id: svn+ssh://svn.code.sf.net/p/migrid/code/trunk@6157 b75ad72c-e7d7-11dd-a971-7dbc132099af
diff --git a/mig/images/js/jquery.filemanager.js b/mig/images/js/jquery.filemanager.js
@@ -1013,6 +1013,61 @@ if (jQuery) (function($){
                   );
         }
 
+        function downloadWrapper(el, dialog, download_url) {
+            var lastinfo = $(statusinfo).html();
+
+            var target = $(el).attr(pathAttribute).split('/');
+            var file_name;
+            if (target.lastIndexOf('/') === target.length-1) {
+                file_name = 'output.bin';
+            } else {
+                file_name = target[target.length-1];
+            }
+            startProgress("Downloading "+file_name);
+
+            $.ajax({
+                url: download_url,
+                dataType: 'binary',
+                method: 'GET',
+                xhrFields: {
+                    responseType: 'blob' // Set the response type to 'blob'
+                },
+                success: function(blob) {
+                    console.info("Starting download of "+file_name)
+                    let fileURL = URL.createObjectURL(blob);
+                    var link = document.createElement('a');
+                    link.href = fileURL;
+                    /* downloaded filename */
+                    link.download = file_name;
+                    document.body.appendChild(link);
+                    link.click();
+                    URL.revokeObjectURL(fileURL);
+                    stopProgress("");
+                },
+                error: function(xhr, textStatus, errorThrown) {
+                    console.error('Error downloading', file_name, ":", textStatus, ":", errorThrown);
+                    //console.info('xhr: ', xhr);
+                    //console.info('xhr.status: ', xhr.status);
+
+                    var hint_txt = "";
+                    /* NOTE: this error code is from cat and in returnvalues */
+                    /* TODO: can we extract actual response from xhr instead? */
+                    if (xhr.status === 422) {
+                        hint_txt = "above web size limit"
+                    } else {
+                        hint_txt = textStatus +" ("+errorThrown+")";
+                    }
+                    var err_msg = "Download failed: "+hint_txt;
+                    stopProgress(err_msg);
+
+                    err_msg += ". Please use other protocols for big downloads.";
+                    $(dialog).dialog(okDialog);
+                    $(dialog).dialog('open');
+                    $(dialog).html(err_msg);
+                }
+            });
+        }
+
         // Callback helpers for context menu
         var callbacks = {
 
@@ -1024,8 +1079,9 @@ if (jQuery) (function($){
                 var path = chroot($(el).attr(pathAttribute), options);
                 if (options.enableGDP) {
                     /* Path may contain URL-unfriendly characters */
-                    document.location = 'cat.py?path=' +
+                    var download_url = 'cat.py?path=' +
                         encodeURIComponent(path)+'&output_format=file';
+                    downloadWrapper(el, '#cmd_dialog', download_url);
                 }
                 else {
                     var path_enc = encodeURI(path);
@@ -1057,8 +1113,9 @@ if (jQuery) (function($){
                     window.open('/cert_redirect/'+path_enc);
                 } else {
                     /* Path may contain URL-unfriendly characters */
-                    document.location = 'cat.py?path=' +
+                    var download_url = 'cat.py?path=' +
                         encodeURIComponent(path)+'&output_format=file';
+                    downloadWrapper(el, '#cmd_dialog', download_url);
                 }
             },
             edit:   function (action, el, pos) {
diff --git a/mig/shared/functionality/cat.py b/mig/shared/functionality/cat.py
@@ -41,7 +41,8 @@
     write_file_lines
 from mig.shared.functional import validate_input_and_cert, REJECT_UNSET
 from mig.shared.handlers import safe_handler, get_csrf_limit
-from mig.shared.init import initialize_main_variables, start_download
+from mig.shared.init import initialize_main_variables, find_entry, \
+    make_start_entry, start_error, start_download
 from mig.shared.parseflags import verbose, binary
 from mig.shared.userio import GDPIOLogError, gdp_iolog
 from mig.shared.safeinput import valid_path_pattern
@@ -112,8 +113,9 @@ def _main(configuration, logger, environ, op_name='', output_objects=None, clien
     if logger is None:
         logger = configuration.logger
 
+    # Create new output_objects list with start entry if None was supplied
     if output_objects is None:
-        output_objects = [] # create a new list if one was not supplied
+        output_objects = [make_start_entry()]
 
     client_dir = client_id_dir(client_id)
     defaults = signature()[1]
@@ -142,6 +144,9 @@ def _main(configuration, logger, environ, op_name='', output_objects=None, clien
     base_dir = os.path.abspath(os.path.join(configuration.user_home,
                                             client_dir)) + os.sep
 
+    output_format = user_arguments_dict.get('output_format', ['txt'])[0]
+    start_entry = find_entry(output_objects, 'start')
+
     if verbose(flags):
         for flag in flags:
             output_objects.append({'object_type': 'text',
@@ -150,28 +155,34 @@ def _main(configuration, logger, environ, op_name='', output_objects=None, clien
     if dst:
         if not safe_handler(configuration, 'post', op_name, client_id,
                             get_csrf_limit(configuration), accepted):
+            status = returnvalues.REJECT_PROCESSING_ERROR
+            error_marker = start_error(configuration, output_format, status)
+            start_entry.update(error_marker)
             output_objects.append(
-                {'object_type': 'error_text', 'text': '''Only accepting
-CSRF-filtered POST requests to prevent unintended updates'''
+                {'object_type': 'error_text', 'text': 'Only accepting '
+                 'CSRF-filtered POST requests to prevent unintended updates'
                  })
-            return (output_objects, returnvalues.CLIENT_ERROR)
+            return (output_objects, status)
 
         # IMPORTANT: path must be expanded to abs for proper chrooting
         abs_dest = os.path.abspath(os.path.join(base_dir, dst))
         relative_dst = abs_dest.replace(base_dir, '')
         if not valid_user_path(configuration, abs_dest, base_dir, True):
             logger.warning('%s tried to %s into restricted path %s ! (%s)'
                            % (client_id, op_name, abs_dest, dst))
+            status = returnvalues.FORBIDDEN_ERROR
+            error_marker = start_error(configuration, output_format, status)
+            start_entry.update(error_marker)
             output_objects.append({'object_type': 'error_text',
                                    'text': "invalid destination: '%s'"
                                    % dst})
-            return (output_objects, returnvalues.CLIENT_ERROR)
+            return (output_objects, status)
 
     src_mode = "rb"
     dst_mode = "wb"
     if binary(flags):
         force_file = True
-    elif user_arguments_dict.get('output_format', ['txt'])[0] == 'file':
+    elif output_format == 'file':
         force_file = True
     else:
         force_file = False
@@ -204,13 +215,17 @@ def _main(configuration, logger, environ, op_name='', output_objects=None, clien
         # (allowed) match
 
         if not match:
+            status = returnvalues.NOT_FOUND_ERROR
+            error_marker = start_error(configuration, output_format, status)
+            start_entry.update(error_marker)
             output_objects.append({'object_type': 'file_not_found',
                                    'name': pattern})
-            status = returnvalues.FILE_NOT_FOUND
 
         if not _check_serve_permitted(configuration, paths=match):
-            status = returnvalues.REJECTED_ERROR
+            status = returnvalues.REJECT_PROCESSING_ERROR
             text = _render_error_text_for_serve_limit(configuration)
+            error_marker = start_error(configuration, output_format, status)
+            start_entry.update(error_marker)
             output_objects.append({'object_type': 'error_text', 'text': text})
             return (output_objects, status)
 
@@ -299,7 +314,7 @@ def _main(configuration, logger, environ, op_name='', output_objects=None, clien
                 if force_file:
                     download_marker = start_download(configuration, abs_path,
                                                      output_lines)
-                    output_objects.append(download_marker)
+                    start_entry.update(download_marker)
                 output_objects.append(entry)
 
     return (output_objects, status)
diff --git a/mig/shared/init.py b/mig/shared/init.py
@@ -4,7 +4,7 @@
 # --- BEGIN_HEADER ---
 #
 # init - shared helpers to init functionality backends
-# Copyright (C) 2003-2023  The MiG Project lead by Brian Vinter
+# Copyright (C) 2003-2024  The MiG Project lead by Brian Vinter
 #
 # This file is part of MiG.
 #
@@ -100,6 +100,22 @@ def start_download(configuration, path, output):
                               % os.path.basename(path))])
 
 
+def start_error(configuration, output_format, status_pair):
+    """Helper to set the headers required to force an error message to be shown
+    in particular in cases where file or binary output delivery was requested,
+    which would hide the error in the file.
+    """
+    _logger = configuration.logger
+    if output_format in ['file', 'binary']:
+        content_type = 'text/plain'
+    else:
+        content_type = output_format
+    _logger.debug('force %s error message for %s request' % (content_type,
+                                                             output_format))
+    return make_start_entry([('Content-Type', content_type),
+                             ('Status', '%s %s' % status_pair)])
+
+
 def initialize_main_variables(client_id, op_title=True, op_header=True,
                               op_menu=True):
     """Script initialization is identical for most scripts in 
diff --git a/mig/shared/output.py b/mig/shared/output.py
@@ -4,7 +4,7 @@
 # --- BEGIN_HEADER ---
 #
 # output - general formatting of backend output objects
-# Copyright (C) 2003-2023  The MiG Project lead by Brian Vinter
+# Copyright (C) 2003-2024  The MiG Project lead by Brian Vinter
 #
 # This file is part of MiG.
 #
@@ -41,6 +41,7 @@
 from mig.shared.defaults import file_dest_sep, keyword_any, keyword_updating
 from mig.shared.htmlgen import get_xgi_html_header, get_xgi_html_footer, \
     vgrid_items, html_post_helper, tablesorter_pager
+from mig.shared.init import find_entry
 from mig.shared.objecttypes import validate
 from mig.shared.prettyprinttable import pprint_table
 from mig.shared.pwcrypto import sorted_hash_algos
@@ -2716,16 +2717,55 @@ def resource_format(configuration, ret_val, ret_msg, out_obj):
 def file_format(configuration, ret_val, ret_msg, out_obj):
     """Dump raw file contents"""
 
+    _logger = configuration.logger
+
     # TODO: use wsgi file_wrapper helper here if out_obj has wsgi entry?
 
     file_content = ''
 
+    # NOTE: carefully handle errors and ONLY render them when proper care has
+    #       been taken to deliver them as actual output, to avoid that they end
+    #       up hidden inside downloaded files.
+    #       Mark error output with the explicit start_error helper in backends
+    #       like it's done in cat.py, and only log such entries as warnings
+    #       here otherwise.
+    render_text, render_errors = False, False
+    headers = []
+    content_type = 'application/octet-stream'
+    start_entry = find_entry(out_obj, 'start')
+    if start_entry is not None:
+        headers = start_entry.get('headers', [])
+    for (key, val) in headers:
+        if key == 'Content-Type':
+            content_type = val
+    if content_type in ('text/plain', 'text/html'):
+        render_text, render_errors = True, True
+    _logger.debug("render output in file_format: %s (%s %s)" %
+                  (out_obj, render_text, render_errors))
     for entry in out_obj:
         if entry['object_type'] == 'file_output':
             for line in entry['lines']:
                 file_content += line
         elif entry['object_type'] == 'binary':
             file_content = entry['data']
+        elif entry['object_type'] == 'text':
+            if render_text:
+                file_content += entry['text']
+            else:
+                _logger.warning("skip %(object_type)s in file_format: %(text)s"
+                                % entry)
+        elif entry['object_type'] == 'error_text':
+            if render_errors:
+                file_content += entry['text']
+            else:
+                _logger.warning("skip %(object_type)s in file_format: %(text)s"
+                                % entry)
+        elif entry['object_type'] == 'file_not_found':
+            if render_errors:
+                file_content += '%(name)s: No such file or directory\n' % entry
+            else:
+                _logger.warning("skip %(object_type)s in file_format: %(name)s"
+                                % entry)
 
     return file_content
 
diff --git a/mig/shared/returnvalues.py b/mig/shared/returnvalues.py
@@ -45,8 +45,11 @@
 SYSTEM_ERROR = (200, 'SYSTEM_ERROR')
 USER_NOT_CREATED = (201, 'USER_NOT_CREATED')
 OUTPUT_VALIDATION_ERROR = (202, 'The output the server '
-                            + 'has generated could not be validated')
+                           + 'has generated could not be validated')
 
 # REQUEST ERRORS
 
-REJECTED_ERROR = (422, 'REJECTED')
+BAD_REQUEST_ERROR = (400, 'Bad Request')
+FORBIDDEN_ERROR = (403, 'Forbidden')
+NOT_FOUND_ERROR = (404, 'Not Found')
+REJECT_PROCESSING_ERROR = (422, 'Unprocessable Content')
diff --git a/tests/test_mig_shared_functionality_cat.py b/tests/test_mig_shared_functionality_cat.py
@@ -109,7 +109,8 @@ def test_file_serving_a_single_file_match(self):
                                            user_arguments_dict=payload,
                                            environ=self.test_environ)
 
-        self.assertEqual(len(output_objects), 1)
+        # NOTE: start entry with headers and actual content
+        self.assertEqual(len(output_objects), 2)
         self.assertSingleOutputObject(output_objects,
                                       with_object_type='file_output')
 
@@ -161,14 +162,14 @@ def test_file_serving_over_limit_without_storage_protocols(self):
                                            user_arguments_dict=payload,
                                            environ=self.test_environ)
 
-        self.assertEqual(len(output_objects), 1)
+        # NOTE: start entry with headers and actual error message
+        self.assertEqual(len(output_objects), 2)
         relevant_obj = self.assertSingleOutputObject(output_objects,
-            with_object_type='error_text')
+                                                     with_object_type='error_text')
         self.assertEqual(relevant_obj['text'],
                          "Site configuration prevents web serving contents "
                          "bigger than 3896 bytes")
 
-
     def test_file_serving_over_limit_with_storage_protocols_sftp(self):
         test_binary_file = os.path.realpath(os.path.join(TEST_DATA_DIR,
                                                          'loading.gif'))
@@ -186,13 +187,13 @@ def test_file_serving_over_limit_with_storage_protocols_sftp(self):
         self.configuration.wwwserve_max_bytes = test_binary_file_size - 1
 
         (output_objects, status) = submain(self.configuration, self.logger,
-                                        client_id=self.TEST_CLIENT_ID,
-                                        user_arguments_dict=payload,
-                                        environ=self.test_environ)
+                                           client_id=self.TEST_CLIENT_ID,
+                                           user_arguments_dict=payload,
+                                           environ=self.test_environ)
 
-        self.assertEqual(len(output_objects), 1)
+        # NOTE: start entry with headers and actual error message
         relevant_obj = self.assertSingleOutputObject(output_objects,
-            with_object_type='error_text')
+                                                     with_object_type='error_text')
         self.assertEqual(relevant_obj['text'],
                          "Site configuration prevents web serving contents "
                          "bigger than 3896 bytes - please use better "
@@ -203,15 +204,17 @@ def test_main_passes_environ(self):
         try:
             result = realmain(self.TEST_CLIENT_ID, {}, None)
         except Exception as unexpectedexc:
-            raise AssertionError("saw unexpected exception: %s" % (unexpectedexc,))
+            raise AssertionError(
+                "saw unexpected exception: %s" % (unexpectedexc,))
 
         (output_objects, status) = result
         self.assertEqual(status[1], 'Client error')
 
         error_text_objects = _only_output_objects(output_objects,
-            with_object_type='error_text')
+                                                  with_object_type='error_text')
         relevant_obj = error_text_objects[2]
-        self.assertEqual(relevant_obj['text'], 'Input arguments were rejected - not allowed for this script!')
+        self.assertEqual(
+            relevant_obj['text'], 'Input arguments were rejected - not allowed for this script!')
 
 
 if __name__ == '__main__':
