Make the File Manager and storage-path tools act on the path that was checked. #10674

In server mode, check_access_permission() validated a path into the
user's storage directory, but several callers then acted on a path built
differently, so the path checked and the path used could name different
files:

- Opening a file in the Query Tool, saving a file, the Google client
  secret and, on Windows, Import/Export Data, Backup and Restore joined
  the path to the storage directory after stripping leading separators,
  so a path checked as inside storage could name any file the pgAdmin
  account can read, or on Windows write outside it.
- Listing a folder decoded the path again after the check, so a folder
  named like an encoded ".." listed any directory on the host.
- Download, delete, rename and the file-exists check appended the path
  to the storage directory without a separator, so where one user's
  storage directory name was a prefix of another's, that user's files
  could be read, deleted or moved; download also checked the path
  appended to itself.
- Creating a folder checked the parent rather than the folder created,
  and upload wrote to a path that was not checked.

check_access_permission() now returns the absolute path it checked, and
every caller acts on that path. Folder paths are decoded once, rename
checks the new name where it is created, and download returns a 404
without the server path for a missing file.

In server mode on POSIX, download, delete, rename and file loads now
open files relative to directories opened from the storage directory
without following symbolic links, as upload and save already did, so a
parent directory swapped for a symbolic link after the check is refused.
The walk follows the path as written: a ".." goes back up past a
directory the walk opened, rather than being collapsed in the text to a
different file from the one the check resolved. It keeps open only the
directories a later ".." can still go back up to, so a path holds at
most two more file descriptors than it has ".." segments, and working
out which to keep takes time linear in the length of the path.

Also:
- The storage directory itself can no longer be deleted.
- In desktop mode, a download refuses a FIFO or other non-regular file
  instead of blocking on it; symbolic links are still followed there.
- A symbolic link refusal is recognised as ELOOP, or EMLINK only where
  O_NOFOLLOW uses it.
- The File Manager sends the selected storage when checking whether a
  file exists, so the overwrite prompt sees files in a shared storage.
- get_closest_parent() tested whether the storage directory existed
  rather than the remembered folder; it now finds the closest existing
  folder inside storage.
- A refused or failed download now shows the server's message.
- check_file_for_bom_and_binary() reports a read error in its result.

web/pgadmin/misc/file_manager/__init__.py

@@ -10,6 +10,7 @@
 """Implements File Manager"""
 
 import errno
+import io
 import os
 import os.path
 import secrets
@@ -26,7 +27,7 @@ import pathlib
 
 import json
 from flask import render_template, Response, session, request as req, \
-    url_for, current_app, send_from_directory
+    url_for, current_app, send_file
 from flask_babel import gettext
 from pgadmin.user_login_check import pga_login_required
 from pgadmin.utils import PgAdminModule
@@ -81,32 +82,78 @@ def _open_parent_beneath(root, path):
     link (with ELOOP, or EMLINK on some BSDs) wherever it appears.
 
     `root` itself is trusted and may be a symbolic link, since it is the
-    storage directory configured by the administrator. `path` is taken
-    lexically, so a symbolic link inside the storage directory is refused
-    even when it points elsewhere inside it.
+    storage directory configured by the administrator. A symbolic link
+    inside the storage directory is refused even when it points elsewhere
+    inside it.
+
+    `path` is walked as written, not normalised first: a ".." goes back up
+    past a directory the walk has opened, so "link/.." opens "link" (and is
+    refused) rather than being collapsed in the text to a directory that
+    check_access_permission(), which resolves the link first, never named.
+    `path` must be written as `root` followed by its components; any other
+    path is refused. The walk never goes above `root`, so this holds for a
+    path that has not been through check_access_permission() too, but
+    callers pass the path that it returned.
 
     Returns (descriptor of the parent directory, name of the leaf); the
     caller must close the descriptor.
     """
-    if not root:
-        raise PermissionError(errno.EACCES, gettext("Access denied"), path)
-    rel = os.path.relpath(os.path.abspath(path), os.path.abspath(root))
-    parts = pathlib.PurePath(rel).parts
-    if not parts or parts == ('.',) or parts[0] == os.pardir:
-        raise PermissionError(errno.EACCES, gettext("Access denied"), path)
+    def denied():
+        return PermissionError(errno.EACCES, gettext("Access denied"), path)
 
-    dir_fd = os.open(os.path.realpath(root),
-                     os.O_RDONLY | os.O_DIRECTORY | os.O_CLOEXEC)
+    def components(p):
+        if not os.path.isabs(p):
+            p = os.path.join(os.getcwd(), p)
+        return [c for c in p.split(os.sep) if c not in ('', os.curdir)]
+
+    if not root:
+        raise denied()
+    root_parts = components(root)
+    parts = components(path)
+    if parts[:len(root_parts)] != root_parts:
+        raise denied()
+    parts = parts[len(root_parts):]
+    if not parts or parts[-1] == os.pardir:
+        raise denied()
+
+    # Keep open only the ancestors a later ".." can still go back up to:
+    # after each directory, as many as there are ".." segments still to
+    # come, plus the current one. A path then holds at most two more
+    # descriptors than it has ".." segments (one more while the next
+    # directory is being opened), rather than one per directory.
+    # `depth` is how far below `root` the walk is, whether or not the
+    # descriptors of the directories above are still held.
+    dirs = parts[:-1]
+    pardirs_after = [0] * len(dirs)
+    remaining = 0
+    for i in range(len(dirs) - 1, -1, -1):
+        pardirs_after[i] = remaining
+        remaining += dirs[i] == os.pardir
+    fds = [os.open(os.path.realpath(root),
+                   os.O_RDONLY | os.O_DIRECTORY | os.O_CLOEXEC)]
+    depth = 0
     try:
-        for name in parts[:-1]:
-            next_fd = _openat_nofollow(dir_fd, name,
-                                       os.O_RDONLY | os.O_DIRECTORY, 0, path)
-            os.close(dir_fd)
-            dir_fd = next_fd
+        for i, name in enumerate(dirs):
+            if name == os.pardir:
+                # Never above the storage directory.
+                if depth == 0:
+                    raise denied()
+                os.close(fds.pop())
+                depth -= 1
+                continue
+            fds.append(_openat_nofollow(fds[-1], name,
+                                        os.O_RDONLY | os.O_DIRECTORY, 0,
+                                        path))
+            depth += 1
+            while len(fds) > pardirs_after[i] + 1:
+                os.close(fds.pop(0))
     except BaseException:
-        os.close(dir_fd)
+        for fd in fds:
+            os.close(fd)
         raise
-    return dir_fd, parts[-1]
+    for fd in fds[:-1]:
+        os.close(fd)
+    return fds[-1], parts[-1]
 
 
 def _open_beneath(root, path, flags, mode=0o777):
@@ -204,13 +251,25 @@ def _open_read_target(path, root):
     between cannot redirect the read outside the storage directory. Returns
     a binary read-mode file object.
 
-    In server mode only a regular file is accepted: the open is
-    non-blocking so that a FIFO planted at the path cannot hang the request
-    waiting for a writer, and anything that is not a regular file is
-    refused with EINVAL.
+    Only a regular file is accepted: the open is non-blocking so that a
+    FIFO at the path cannot hang the request waiting for a writer, and
+    anything that is not a regular file is refused with EINVAL. In desktop
+    mode symbolic links are followed, as there is no storage directory to
+    escape.
     """
     if not _refuses_leaf_symlinks():
-        return open(path, 'rb')
+        nonblock = getattr(os, 'O_NONBLOCK', 0)
+        fd = os.open(path, os.O_RDONLY | nonblock | getattr(os, 'O_BINARY', 0))
+        try:
+            if not stat.S_ISREG(os.fstat(fd).st_mode):
+                raise OSError(errno.EINVAL, gettext("Not a regular file"),
+                              path)
+            if nonblock:
+                os.set_blocking(fd, True)
+        except BaseException:
+            os.close(fd)
+            raise
+        return os.fdopen(fd, 'rb')
 
     fd = _open_beneath(root, path, os.O_RDONLY | os.O_NONBLOCK)
     try:
@@ -234,6 +293,67 @@ def _open_read_target(path, root):
     return os.fdopen(fd, 'rb')
 
 
+def _is_nofollow_refusal(e):
+    """True if the OSError `e` is the errno O_NOFOLLOW refuses a symbolic
+    link with: ELOOP, or EMLINK on the BSDs that use it. On Linux EMLINK is
+    only ever a real "too many links" error (e.g. renaming a directory into
+    one at its link limit), so it is not read as a refusal there."""
+    return e.errno == errno.ELOOP or (
+        e.errno == errno.EMLINK and not _platform.startswith('linux'))
+
+
+def _acts_beneath():
+    """True when `_remove_beneath()` and `_rename_beneath()` act relative
+    to directories opened by `_open_parent_beneath()`."""
+    return _refuses_leaf_symlinks() and \
+        {os.rmdir, os.unlink, os.rename} <= os.supports_dir_fd
+
+
+def _remove_beneath(path, root):
+    """Remove the file or empty directory `path`, which must lie inside
+    `root`, with the same protection as `_open_read_target()`: in server
+    mode a symbolic link swapped in for a parent directory since the check
+    is refused rather than followed, and a symbolic link at the leaf is
+    removed itself.
+    """
+    if not _acts_beneath():
+        if os.path.isdir(path):
+            os.rmdir(path)
+        else:
+            os.remove(path)
+        return
+
+    dir_fd, name = _open_parent_beneath(root, path)
+    try:
+        st = os.stat(name, dir_fd=dir_fd, follow_symlinks=False)
+        if stat.S_ISDIR(st.st_mode):
+            os.rmdir(name, dir_fd=dir_fd)
+        else:
+            os.unlink(name, dir_fd=dir_fd)
+    finally:
+        os.close(dir_fd)
+
+
+def _rename_beneath(old, new, root):
+    """Rename `old` to `new`, which must both lie inside `root`, with the
+    same protection as `_remove_beneath()`. A symbolic link at either leaf
+    is renamed or replaced itself, never followed."""
+    if not _acts_beneath():
+        os.rename(old, new)
+        return
+
+    old_fd, old_name = _open_parent_beneath(root, old)
+    try:
+        new_fd, new_name = _open_parent_beneath(root, new)
+        try:
+            os.rename(old_name, new_name, src_dir_fd=old_fd,
+                      dst_dir_fd=new_fd)
+        finally:
+            os.close(new_fd)
+    finally:
+        os.close(old_fd)
+
+
 def _refuses_leaf_symlinks():
     """True when `_open_upload_target()` and `_open_read_target()` open
     through `_open_beneath()`, refusing symbolic links.
@@ -306,13 +426,23 @@ def is_folder_hidden(filepath):
         return os.path.basename(filepath).startswith('.')
 
 
-def read_file_generator(file, enc):
+def _open_text(file, root, **kwargs):
+    """Open `file` for reading as text. With a storage directory `root`, it
+    is opened with `_open_read_target()`, so a symbolic link swapped in
+    since the path was checked is not followed."""
+    if root is None:
+        return open(file, 'r', **kwargs)
+    return io.TextIOWrapper(_open_read_target(file, root), **kwargs)
+
+
+def read_file_generator(file, enc, root=None):
     """
     This will read the content of the file selected by user.
 
     Args:
         file: Path to the file to be read.
         enc: Initial encoding to attempt for reading the file.
+        root: The storage directory `file` was checked against, if any.
 
     Returns:
         Generator yielding 4MB chunks of the file content.
@@ -321,7 +451,8 @@ def read_file_generator(file, enc):
     errors = None
     if enc != 'latin-1':
         try:
-            with open(file, 'r', encoding=enc, newline='') as fileObj:
+            with _open_text(file, root, encoding=enc,
+                            newline='') as fileObj:
                 while True:
                     data = fileObj.read(4194304)
                     if not data:
@@ -351,7 +482,7 @@ def read_file_generator(file, enc):
     if errors is not None:
         open_kwargs['errors'] = errors
 
-    with open(file, 'r', **open_kwargs) as fileObj:
+    with _open_text(file, root, **open_kwargs) as fileObj:
         while True:
             # 4MB chunk (4 * 1024 * 1024 Bytes)
             data = fileObj.read(4194304)
@@ -572,33 +703,31 @@ def load_file():
         shared_storage=storage_folder)
 
     try:
-        Filemanager.check_access_permission(storage_manager_path, file_path)
+        checked_path = Filemanager.check_access_permission(
+            storage_manager_path, file_path)
     except Exception as e:
         return internal_server_error(errormsg=str(e))
 
     if storage_manager_path:
-        # generate full path of file
-        file_path = os.path.join(
-            storage_manager_path,
-            file_path.lstrip('/').lstrip('\\')
-        )
+        # The full path of the file is the one that was checked.
+        file_path = checked_path
 
     (status, err_msg, is_binary,
      is_startswith_bom, enc) = Filemanager.check_file_for_bom_and_binary(
-        file_path
+        file_path, root=storage_manager_path
     )
 
     if not status:
-        return internal_server_error(
-            errormsg=gettext(err_msg)
-        )
+        # Already translated, or the operating system's own message.
+        return internal_server_error(errormsg=err_msg)
 
     if is_binary:
         return internal_server_error(
             errormsg=gettext("File type not supported")
         )
 
-    return Response(read_file_generator(file_path, enc), mimetype='text/plain')
+    return Response(read_file_generator(file_path, enc, storage_manager_path),
+                    mimetype='text/plain')
 
 
 @blueprint.route('/save_file/', methods=["PUT", "POST"],
@@ -636,15 +765,14 @@ def save_file():
     file_path = unquote(file_data['file_name'])
 
     try:
-        Filemanager.check_access_permission(storage_manager_path, file_path)
+        checked_path = Filemanager.check_access_permission(
+            storage_manager_path, file_path)
     except Exception as e:
         return internal_server_error(errormsg=str(e))
 
     if storage_manager_path is not None:
-        file_path = os.path.join(
-            storage_manager_path,
-            file_path.lstrip('/').lstrip('\\')
-        )
+        # The full path of the file is the one that was checked.
+        file_path = checked_path
 
     # Save in the encoding the file was loaded with, if it was loaded.
     enc = Filemanager.get_file_encoding(file_path)
@@ -671,7 +799,7 @@ def save_file():
         # actually set, though: otherwise the errno is a real filesystem
         # error (a symlink loop, say) and the raw strerror is the truth.
         if _refuses_leaf_symlinks() and \
-                e.errno in (errno.ELOOP, errno.EMLINK):
+                _is_nofollow_refusal(e):
             return internal_server_error(errormsg=gettext(
                 "Refusing to write through a symbolic link."))
         err_msg = error_str.format(e.strerror)
@@ -723,12 +851,26 @@ class Filemanager():
         if len(last_dir) > 1 and \
                 (last_dir.endswith('/') or last_dir.endswith('\\')):
             last_dir = last_dir[:-1]
+
+        def exists_in_storage(path):
+            # A folder outside the storage directory counts as missing, so
+            # its existence is never tested or revealed.
+            # A remembered folder the path cannot even be resolved for
+            # (e.g. one with a NUL byte) counts as missing too.
+            try:
+                abs_path = Filemanager.check_access_permission(
+                    storage_dir, path)
+            except (OSError, ValueError):
+                return False
+            return os.path.exists(abs_path)
+
         while last_dir:
-            if os.path.exists(storage_dir or '' + last_dir):
+            if exists_in_storage(last_dir):
                 break
             index = max(last_dir.rfind('\\'), last_dir.rfind('/')) \
                 if _platform == 'win32' else last_dir.rfind('/')
-            last_dir = last_dir[0:index]
+            # Without a separator there is no parent left to try.
+            last_dir = last_dir[0:index] if index >= 0 else ''
 
         if _platform == 'win32':
             if not last_dir.endswith('\\'):
@@ -1102,7 +1244,6 @@ class Filemanager():
         files_only = trans_data.get('files_only', '')
         supported_types = trans_data.get('supported_types', [])
 
-        orig_path = unquote(orig_path)
         try:
             files = Filemanager.get_files_in_path(
                 is_show_hidden_files, files_only, folders_only,
@@ -1120,8 +1261,17 @@ class Filemanager():
 
     @staticmethod
     def check_access_permission(in_dir, path, skip_permission_check=False):
+        """Check that `path`, relative to the storage directory `in_dir`,
+        stays inside it in server mode, and return the absolute path that
+        was checked.
+
+        Callers must do the file operation on the returned path rather
+        than build their own from `in_dir` and `path`: any other way of
+        joining them can name a different file from the one checked.
+        """
+        abs_path = Filemanager.get_abs_path(in_dir, path)
         if not config.SERVER_MODE or skip_permission_check:
-            return
+            return abs_path
 
         in_dir = '' if in_dir is None else in_dir
         orig_path = Filemanager.get_abs_path(in_dir, path)
@@ -1151,6 +1301,8 @@ class Filemanager():
         except ValueError:
             raise PermissionError(gettext("Access denied ({0})").format(path))
 
+        return abs_path
+
     @staticmethod
     def get_abs_path(in_dir, path):
 
@@ -1233,9 +1385,6 @@ class Filemanager():
         else:
             the_dir = self.dir if self.dir is not None else ''
 
-        Filemanager.check_access_permission(the_dir, old)
-        Filemanager.check_access_permission(the_dir, new)
-
         # check if it's dir
         if old[-1] == '/':
             old = old[:-1]
@@ -1251,13 +1400,17 @@ class Filemanager():
         newname = new
         newpath = path + newname
 
-        # make system old path
-        oldpath_sys = "{0}{1}".format(the_dir, old)
-        newpath_sys = "{0}{1}".format(the_dir, newpath)
+        # Check the paths that are renamed: the new name is relative to the
+        # old one's directory, not to the storage directory.
+        oldpath_sys = Filemanager.check_access_permission(the_dir, old)
+        newpath_sys = Filemanager.check_access_permission(the_dir, newpath)
 
         try:
-            os.rename(oldpath_sys, newpath_sys)
+            _rename_beneath(oldpath_sys, newpath_sys, the_dir)
         except OSError as e:
+            if _acts_beneath() and _is_nofollow_refusal(e):
+                return internal_server_error(gettext(
+                    "Refusing to rename through a symbolic link."))
             return internal_server_error("{0} {1}".format(
                 gettext('There was an error renaming the file:'), e.strerror))
 
@@ -1278,16 +1431,20 @@ class Filemanager():
             the_dir = self.shared_dir
         else:
             the_dir = self.dir if self.dir is not None else ''
-        orig_path = "{0}{1}".format(the_dir, path)
+        orig_path = Filemanager.check_access_permission(the_dir, path)
 
-        Filemanager.check_access_permission(the_dir, path)
+        # The storage directory itself lies "inside" it for the check, but
+        # is not the user's to delete.
+        if config.SERVER_MODE and \
+                os.path.realpath(orig_path) == os.path.realpath(the_dir):
+            raise PermissionError(gettext("Access denied ({0})").format(path))
 
         try:
-            if os.path.isdir(orig_path):
-                os.rmdir(orig_path)
-            else:
-                os.remove(orig_path)
+            _remove_beneath(orig_path, the_dir)
         except OSError as e:
+            if _acts_beneath() and _is_nofollow_refusal(e):
+                return internal_server_error(gettext(
+                    "Refusing to delete through a symbolic link."))
             return internal_server_error("{0} {1}".format(
                 gettext('There was an error deleting the file:'), e.strerror))
 
@@ -1310,19 +1467,12 @@ class Filemanager():
 
             file_obj = req.files['newfile']
             file_name = file_obj.filename
-            orig_path = "{0}{1}".format(the_dir, path)
-            new_name = "{0}{1}".format(orig_path, file_name)
-
+            # Check the new file's path is inside the user's storage
+            # directory, and write to the path that was checked.
             try:
-                # Check the new file's resolved path is inside the user's
-                # storage directory. realpath() resolves symbolic links —
-                # abspath() alone leaves a CWE-61 escape window.
-                if config.SERVER_MODE:
-                    real_target = os.path.realpath(
-                        os.path.join(the_dir, new_name))
-                    real_in_dir = os.path.realpath(the_dir)
-                    pathlib.Path(real_target).relative_to(real_in_dir)
-            except ValueError:
+                new_name = Filemanager.check_access_permission(
+                    the_dir, "{0}{1}".format(path, file_name))
+            except PermissionError:
                 return unauthorized(self.ERROR_NOT_ALLOWED['Error'])
 
             with _open_upload_target(new_name, the_dir) as f:
@@ -1348,17 +1498,17 @@ class Filemanager():
         """
         Checks whether given file exists or not
         """
-        the_dir = self.dir if self.dir is not None else ''
+        if self.shared_dir:
+            the_dir = self.shared_dir
+        else:
+            the_dir = self.dir if self.dir is not None else ''
         code = 1
 
         name = unquote(name)
         path = unquote(path)
 
-        orig_path = "{0}{1}".format(the_dir, path)
-        Filemanager.check_access_permission(
+        new_name = Filemanager.check_access_permission(
             the_dir, "{}{}".format(path, name))
-
-        new_name = "{0}{1}".format(orig_path, name)
         if not os.path.exists(new_name):
             code = 0
 
@@ -1402,7 +1552,7 @@ class Filemanager():
             os.path.realpath(filename), default)
 
     @staticmethod
-    def check_file_for_bom_and_binary(filename, enc="utf-8"):
+    def check_file_for_bom_and_binary(filename, enc="utf-8", root=None):
         """
         This utility function will check if file is Binary file
         and/or if it startswith BOM character
@@ -1410,6 +1560,8 @@ class Filemanager():
         Args:
             filename: File
             enc: Encoding for the file
+            root: The storage directory `filename` was checked against, if
+                any; the file is then opened with `_open_read_target()`
 
         Returns:
             Status(Error?), Error message, Binary file flag,
@@ -1434,7 +1586,8 @@ class Filemanager():
         # read the file
         try:
 
-            with open(filename, 'rb') as f:
+            with (open(filename, 'rb') if root is None
+                  else _open_read_target(filename, root)) as f:
                 file_data = f.read(1024)
 
             # Check for BOM in file data
@@ -1457,14 +1610,18 @@ class Filemanager():
 
         except IOError as ex:
             # we don't want to expose real path of file
-            # so only show error message.
-            if ex.strerror == 'Permission denied':
-                return unauthorized(str(ex.strerror))
+            # so only show error message. Callers unpack the result, so
+            # report the failure in it rather than returning a response.
+            status = False
+            if root is not None and _refuses_leaf_symlinks() and \
+                    _is_nofollow_refusal(ex):
+                err_msg = gettext("Refusing to read through a symbolic link.")
             else:
-                return internal_server_error(str(ex.strerror))
+                err_msg = str(ex.strerror)
 
         except Exception as ex:
-            return internal_server_error(str(ex.strerror))
+            status = False
+            err_msg = str(getattr(ex, 'strerror', None) or ex)
 
         # Remove root storage path from error message
         # when running in Server mode
@@ -1487,11 +1644,14 @@ class Filemanager():
         else:
             user_dir = self.dir if self.dir is not None else ''
 
-        Filemanager.check_access_permission(user_dir, "{}{}".format(
+        # Check the folder that will be created: get_new_name() puts a
+        # separator between `path` and `name` that a check of the two
+        # joined as they are would not see.
+        Filemanager.check_access_permission(user_dir, "{}/{}".format(
             path, name))
-
         create_path, new_path, new_name = \
             self.get_new_name(user_dir, path, name)
+        create_path = Filemanager.check_access_permission(user_dir, new_path)
         try:
             os.mkdir(create_path)
         except OSError as e:
@@ -1518,27 +1678,42 @@ class Filemanager():
         else:
             the_dir = self.dir if self.dir is not None else ''
 
-        orig_path = "{0}{1}".format(the_dir, path)
-
-        Filemanager.check_access_permission(
-            the_dir, "{}{}".format(path, path)
-        )
+        # Open the path that was checked without following a symbolic link
+        # swapped in since the check.
+        orig_path = Filemanager.check_access_permission(the_dir, path)
 
         filename = os.path.basename(path)
-        if orig_path and len(orig_path) > 0:
-            dir_path = os.path.dirname(orig_path)
-        else:
-            dir_path = os.path.dirname(path)
+        try:
+            download_file = _open_read_target(orig_path, the_dir)
+        except FileNotFoundError:
+            return make_json_response(
+                status=404,
+                errormsg=gettext("'{0}' file does not exist.").format(path))
+        except OSError as e:
+            if _refuses_leaf_symlinks() and \
+                    _is_nofollow_refusal(e):
+                return internal_server_error(errormsg=gettext(
+                    "Refusing to read through a symbolic link."))
+            return internal_server_error(errormsg=e.strerror)
 
-        response = send_from_directory(dir_path, filename,
-                                       mimetype='application/octet-stream',
-                                       as_attachment=True)
+        # A file object gives send_file() no size or modification time, so
+        # set them from the descriptor; range requests are not offered.
+        st = os.fstat(download_file.fileno())
+        response = send_file(download_file,
+                             mimetype='application/octet-stream',
+                             as_attachment=True, download_name=filename,
+                             conditional=False, etag=False,
+                             last_modified=st.st_mtime)
+        response.content_length = st.st_size
         response.headers["filename"] = filename
 
         return response
 
     def permission(self, path=None):
-        the_dir = self.dir if self.dir is not None else ''
+        if self.shared_dir:
+            the_dir = self.shared_dir
+        else:
+            the_dir = self.dir if self.dir is not None else ''
         res = {'Code': 1}
         Filemanager.check_access_permission(the_dir, path)
         return res

web/pgadmin/misc/file_manager/static/js/components/FileManager.jsx

@@ -305,16 +305,36 @@ export class FileManagerUtils {
   }
 
   async downloadFile(row, ss) {
-    let res = await this.api({
-      method: 'POST',
-      url: this.fileConnectorUrl,
-      responseType: 'blob',
-      data: {
-        'mode': 'download',
-        'path': row.Path,
-        'storage_folder': ss,
-      },
-    });
+    let res;
+    try {
+      res = await this.api({
+        method: 'POST',
+        url: this.fileConnectorUrl,
+        responseType: 'blob',
+        data: {
+          'mode': 'download',
+          'path': row.Path,
+          'storage_folder': ss,
+        },
+      });
+    } catch (error) {
+      // An error is sent as JSON but arrives as a Blob, since the request
+      // asks for one; read it back so the message can be shown.
+      if(error.response?.data instanceof Blob) {
+        try {
+          const text = await new Promise((resolve, reject)=>{
+            const reader = new FileReader();
+            reader.onload = ()=>resolve(reader.result);
+            reader.onerror = ()=>reject(reader.error);
+            reader.readAsText(error.response.data);
+          });
+          error.response.data = JSON.parse(text);
+        } catch {
+          /* Not JSON; leave it for the status text to be shown */
+        }
+      }
+      throw error;
+    }
     DownloadUtils.downloadBlob(res.data, res.headers.filename);
   }
 
@@ -362,6 +382,7 @@ export class FileManagerUtils {
       'path': path,
       'name': fileName,
       'mode': 'is_file_exist',
+      'storage_folder': this.storage_folder,
     });
     return Boolean(res.data.data.result.Code);
   }

web/pgadmin/utils/__init__.py

@@ -806,10 +806,12 @@ def filename_with_file_manager_path(_file, create_file=False,
                                   status=403)
 
     from pgadmin.misc.file_manager import Filemanager
-    Filemanager.check_access_permission(
+    checked_path = Filemanager.check_access_permission(
         storage_dir, _file, skip_permission_check)
     if storage_dir:
-        _file = os.path.join(storage_dir, _file.lstrip('/').lstrip('\\'))
+        # Use the path that was checked: joining storage_dir and _file any
+        # other way can name a different file.
+        _file = checked_path
     elif not os.path.isabs(_file):
         _file = os.path.join(document_dir(), _file)
 
@@ -1646,9 +1648,9 @@ def _is_refused_leaf_symlink(e):
     """True if `e` is the kernel refusing to follow a symbolic link because
     the file was opened with O_NOFOLLOW (ELOOP, or EMLINK on some BSDs),
     rather than a genuine filesystem error."""
-    from pgadmin.misc.file_manager import _refuses_leaf_symlinks
-    return _refuses_leaf_symlinks() and \
-        e.errno in (errno.ELOOP, errno.EMLINK)
+    from pgadmin.misc.file_manager import _refuses_leaf_symlinks, \
+        _is_nofollow_refusal
+    return _refuses_leaf_symlinks() and _is_nofollow_refusal(e)
 
 
 def _handle_error(error_msg, from_setup):

web/pgadmin/utils/tests/test_open_utility_file.py

@@ -25,6 +25,7 @@ from unittest.mock import patch
 import config
 from pgadmin.utils import open_utility_file, _is_refused_leaf_symlink, \
     filename_with_file_manager_path
+from pgadmin.misc.file_manager import Filemanager
 from pgadmin.utils.route import BaseTestGenerator
 
 
@@ -83,9 +84,12 @@ class _UtilityFileMixin:
 
     def _call(self, name, plant_link_target=None):
         def plant(storage_dir, file, skip=False):
+            # Stands in for check_access_permission(), so returns the path
+            # to act on as it does.
             if plant_link_target is not None:
                 os.symlink(plant_link_target,
                            os.path.join(storage_dir, file.lstrip('/')))
+            return Filemanager.get_abs_path(storage_dir, file)
 
         with patch('pgadmin.utils.get_storage_directory',
                    return_value=self.storage), \
@@ -463,6 +467,7 @@ class TestParentSymlinkTouchRefused(_UtilityFileMixin, BaseTestGenerator):
         def swap(storage_dir, file, skip=False):
             os.rename(sub, sub + '.moved')
             os.symlink(self.outside, sub)
+            return Filemanager.get_abs_path(storage_dir, file)
 
         with patch('pgadmin.misc.file_manager.Filemanager.'
                    'check_access_permission', side_effect=swap), \

web/regression/javascript/file_manager/FileManager.spec.js

@@ -15,7 +15,7 @@ import Theme from '../../../pgadmin/static/js/Theme';
 import FileManager, { FileManagerUtils, getComparator } from '../../../pgadmin/misc/file_manager/static/js/components/FileManager';
 import MockAdapter from 'axios-mock-adapter';
 import axios from 'axios';
-import getApiInstance from '../../../pgadmin/static/js/api_instance';
+import getApiInstance, { parseApiError } from '../../../pgadmin/static/js/api_instance';
 import DownloadUtils from '../../../pgadmin/static/js/DownloadUtils';
 import userEvent from '@testing-library/user-event';
 
@@ -237,7 +237,10 @@ describe('FileManagerUtils', ()=>{
         retVal = 'blobdata';
         headers = {filename: 'newfile1'};
       } else if(apiData.mode == 'is_file_exist') {
-        retVal = {data: {result: {Code: 1}}};
+        // Only the shared storage named in the request has the file.
+        retVal = {data: {result: {
+          Code: apiData.storage_folder == 'shared1' ? 1 : 0,
+        }}};
       }
       return [200, retVal, headers];
     });
@@ -340,6 +343,12 @@ describe('FileManagerUtils', ()=>{
   });
 
   it('isFileExists', async ()=>{
+    let res = await fmObj.isFileExists('/home/newfolder', 'newfile1');
+    expect(res).toBe(false);
+  });
+
+  it('isFileExists looks in the selected storage', async ()=>{
+    fmObj.storage_folder = 'shared1';
     let res = await fmObj.isFileExists('/home/newfolder', 'newfile1');
     expect(res).toBe(true);
   });
@@ -350,4 +359,18 @@ describe('FileManagerUtils', ()=>{
     await fmObj.downloadFile(row);
     expect(DownloadUtils.downloadBlob).toHaveBeenCalledWith('blobdata', 'newfile1');
   });
+
+  it('downloadFile reports the error the server sent', async ()=>{
+    // A refused download is sent as JSON but arrives as a Blob, since the
+    // request asks for one.
+    networkMock.onPost(`/file_manager/filemanager/${transId}/`).reply(()=>[
+      404,
+      new Blob([JSON.stringify({errormsg: 'File does not exist.'})],
+        {type: 'application/json'}),
+      {'content-type': 'application/json'},
+    ]);
+    let error = await fmObj.downloadFile(
+      {Filename: 'gone', Path: '/home/gone'}).catch((e)=>e);
+    expect(parseApiError(error)).toBe('File does not exist.');
+  });
 });