diff --git a/fileglancer/app.py b/fileglancer/app.py index d3a01aa2..711b0337 100644 --- a/fileglancer/app.py +++ b/fileglancer/app.py @@ -1274,27 +1274,28 @@ async def get_file_metadata(path_name: str, subpath: Optional[str] = Query(''), raise HTTPException(status_code=404 if "not found" in error else 500, detail=error) try: - file_info = filestore.get_file_info(subpath, username) - logger.trace(f"File info: {file_info}") - - result = {"info": json.loads(file_info.model_dump_json())} - - if file_info.is_dir: - try: - files = list(filestore.yield_file_infos(subpath, username)) - result["files"] = [json.loads(f.model_dump_json()) for f in files] - except PermissionError: - logger.error(f"Permission denied when listing files in directory: {subpath}") - result["files"] = [] - result["error"] = "Permission denied when listing directory contents" - return JSONResponse(content=result, status_code=403) - except FileNotFoundError: - logger.error(f"Directory not found during listing: {subpath}") - result["files"] = [] - result["error"] = "Directory contents not found" - return JSONResponse(content=result, status_code=404) - - return result + with db.get_db_session(settings.db_url) as session: + file_info = filestore.get_file_info(subpath, current_user=username, session=session) + logger.trace(f"File info: {file_info}") + + result = {"info": json.loads(file_info.model_dump_json())} + + if file_info.is_dir: + try: + files = list(filestore.yield_file_infos(subpath, current_user=username, session=session)) + result["files"] = [json.loads(f.model_dump_json()) for f in files] + except PermissionError: + logger.error(f"Permission denied when listing files in directory: {subpath}") + result["files"] = [] + result["error"] = "Permission denied when listing directory contents" + return JSONResponse(content=result, status_code=403) + except FileNotFoundError: + logger.error(f"Directory not found during listing: {subpath}") + result["files"] = [] + result["error"] = "Directory contents not found" + return JSONResponse(content=result, status_code=404) + + return result except RootCheckError as e: # Path attempts to escape root directory - try to find a valid fsp for this absolute path diff --git a/fileglancer/filestore.py b/fileglancer/filestore.py index 32f45341..6c3d42a4 100644 --- a/fileglancer/filestore.py +++ b/fileglancer/filestore.py @@ -13,7 +13,9 @@ from typing import Optional, Generator from loguru import logger +from .database import find_fsp_from_absolute_path from .model import FileSharePath +from .utils import is_likely_binary # Default buffer size for streaming file contents DEFAULT_BUFFER_SIZE = 8192 @@ -44,16 +46,107 @@ class FileInfo(BaseModel): last_modified: Optional[float] = None hasRead: Optional[bool] = None hasWrite: Optional[bool] = None + is_symlink: bool = False + symlink_target_fsp: Optional[dict] = None # {"fsp_name": str, "subpath": str} + + @staticmethod + def _safe_readlink(path: str, root_path: Optional[str] = None) -> Optional[str]: + """ + Safely read a symlink target. + + This wrapper centralizes symlink reading with defense-in-depth validation. + When root_path is provided, verifies the symlink's parent directory is + within the allowed root before calling os.readlink(). This check uses + the parent directory (not realpath of the symlink itself) because + realpath would resolve the symlink to its target, which may legitimately + be outside root for cross-share symlinks. + """ + try: + if root_path is not None: + root_real = os.path.realpath(root_path) + # Check the symlink's parent directory is within root + # (don't resolve the symlink itself - that would check the target) + parent_real = os.path.realpath(os.path.dirname(path)) + if not (parent_real == root_real or parent_real.startswith(root_real + os.sep)): + logger.warning(f"Refusing to read symlink outside root: {path}") + return None + return os.readlink(path) + except OSError as e: + logger.warning(f"Failed to read symlink target for {path}: {e}") + return None + + @classmethod + def _get_symlink_target_fsp(cls, absolute_path: str, is_symlink: bool, session, + root_path: Optional[str]) -> Optional[dict]: + """ + Resolve a symlink target to a file share path. + + Returns a dict with fsp_name and subpath if the target is in a known file share, + or None if not a symlink, target not found, or target not in any file share. + """ + if not is_symlink or session is None: + return None + + # Read the symlink target safely + target = cls._safe_readlink(absolute_path, root_path=root_path) + if target is None: + return None + + # Resolve to absolute path (relative symlinks need dirname context) + if not os.path.isabs(target): + target = os.path.join(os.path.dirname(absolute_path), target) + target = os.path.abspath(target) + + # Try to find which file share contains this target + try: + match = find_fsp_from_absolute_path(session, target) + if match: + fsp, subpath = match + + # Reconstruct the canonical target path from the file share root + # and the returned subpath so we only operate within managed roots. + if subpath: + validated_target = os.path.realpath(os.path.join(fsp.mount_path, subpath)) + else: + validated_target = os.path.realpath(fsp.mount_path) + + # Check if the symlink target actually exists within the resolved location. + # If it doesn't exist, return None (broken symlink) + if not os.path.exists(validated_target): + return None + + return {"fsp_name": fsp.name, "subpath": subpath} + except (FileNotFoundError, PermissionError, OSError): + # Target doesn't exist or isn't accessible + pass + + return None @classmethod - def from_stat(cls, path: str, absolute_path: str, stat_result: os.stat_result, current_user: str = None): - """Create FileInfo from os.stat_result""" + def from_stat(cls, path: str, absolute_path: str, + lstat_result: os.stat_result, stat_result: os.stat_result, + current_user: str = None, session = None, + root_path: Optional[str] = None): + """ + Create FileInfo from pre-computed stat results. + + Args: + path: Relative path within the filestore. + absolute_path: Absolute filesystem path (used for basename and symlink resolution). + lstat_result: Result of os.lstat() on the path (detects symlinks). + stat_result: Result of os.stat() or lstat for broken symlinks. + current_user: Username for permission checking (optional). + session: Database session for symlink resolution (optional). + root_path: Filestore root for defense-in-depth validation in symlink reading (optional). + """ if path is None or path == "": raise ValueError("Path cannot be None or empty") + + is_symlink = stat.S_ISLNK(lstat_result.st_mode) is_dir = stat.S_ISDIR(stat_result.st_mode) size = 0 if is_dir else stat_result.st_size # Do not expose the name of the root directory - name = '' if path=='.' else os.path.basename(absolute_path) + name = '' if path == '.' else os.path.basename(absolute_path) permissions = stat.filemode(stat_result.st_mode) last_modified = stat_result.st_mtime @@ -76,6 +169,9 @@ def from_stat(cls, path: str, absolute_path: str, stat_result: os.stat_result, c hasRead = cls._has_read_permission(stat_result, current_user, owner, group) hasWrite = cls._has_write_permission(stat_result, current_user, owner, group) + # Resolve symlink target to file share path if applicable + symlink_target_fsp = cls._get_symlink_target_fsp(absolute_path, is_symlink, session, root_path) + return cls( name=name, path=path, @@ -87,7 +183,9 @@ def from_stat(cls, path: str, absolute_path: str, stat_result: os.stat_result, c group=group, last_modified=last_modified, hasRead=hasRead, - hasWrite=hasWrite + hasWrite=hasWrite, + is_symlink=is_symlink, + symlink_target_fsp=symlink_target_fsp ) @staticmethod @@ -190,19 +288,76 @@ def _check_path_in_root(self, path: Optional[str]) -> str: return full_path - def _get_file_info_from_path(self, full_path: str, current_user: str = None) -> FileInfo: + def _get_file_info_from_path(self, full_path: str, current_user: str = None, session = None) -> FileInfo: """ Get the FileInfo for a file or directory at the given path. + + full_path comes from either: + 1. _check_path_in_root() which validates user input against the root + 2. os.path.join(verified_directory, entry) where entry is from os.listdir() + + In both cases, the path has been validated or constructed from validated + components. We pass full_path (not realpath) to from_stat so that lstat() + can detect symlinks. Symlink targets may be outside the root (cross-fileshare + symlinks), which is valid - we detect and report them without following. + + All filesystem I/O (lstat/stat) is performed here rather than in + FileInfo.from_stat so that path validation and I/O are in the same + method, which allows static analysis tools (CodeQL) to see that the + path is sanitized before use. """ - stat_result = os.stat(full_path) - # Use real paths to avoid /var vs /private/var mismatches on macOS. root_real = os.path.realpath(self.root_path) + + # Defense-in-depth: normalize full_path with abspath (resolves ".." + # without following symlinks) and verify it is within root before any + # filesystem operations. We use abspath (not realpath) because symlinks + # may legitimately point to targets outside root for cross-share links. + full_path = os.path.abspath(full_path) + + def _is_within_root(p: str) -> bool: + return p == root_real or p.startswith(root_real + os.sep) + + # Check the normalized path string is under root (catches .. traversal) + if not _is_within_root(full_path): + raise RootCheckError( + f"Path ({full_path}) is outside root directory ({root_real})", + full_path, + ) + + # Check the resolved parent is under root (catches symlink-based traversal + # e.g. /root/data/symlink_to_etc/passwd where symlink_to_etc -> /etc) + # Skip when full_path is the root itself, since root's parent is above root. + if full_path != root_real: + parent_real = os.path.realpath(os.path.dirname(full_path)) + if not _is_within_root(parent_real): + raise RootCheckError( + f"Path ({full_path}) resolves outside root directory ({root_real})", + full_path, + ) + full_real = os.path.realpath(full_path) if full_real == root_real: rel_path = '.' else: rel_path = os.path.relpath(full_real, root_real) - return FileInfo.from_stat(rel_path, full_path, stat_result, current_user) + + # Perform all filesystem stat calls here, after validation. + lstat_result = os.lstat(full_path) + is_symlink = stat.S_ISLNK(lstat_result.st_mode) + if is_symlink: + try: + stat_result = os.stat(full_path) + except (FileNotFoundError, PermissionError, OSError) as e: + logger.warning(f"Broken symlink detected: {full_path}: {e}") + stat_result = lstat_result + else: + stat_result = os.stat(full_path) + + return FileInfo.from_stat( + rel_path, full_path, lstat_result, stat_result, + current_user=current_user, session=session, + root_path=self.root_path, + ) def get_root_path(self) -> str: @@ -228,7 +383,7 @@ def get_absolute_path(self, relative_path: Optional[str] = None) -> str: return os.path.abspath(os.path.join(self.root_path, relative_path)) - def get_file_info(self, path: Optional[str] = None, current_user: str = None) -> FileInfo: + def get_file_info(self, path: Optional[str] = None, current_user: str = None, session = None) -> FileInfo: """ Get the FileInfo for a file or directory at the given path. @@ -237,12 +392,51 @@ def get_file_info(self, path: Optional[str] = None, current_user: str = None) -> May be None, in which case the root directory is used. current_user (str): The username of the current user for permission checking. May be None, in which case hasRead and hasWrite will be None. + session: Database session for symlink resolution. + May be None, in which case symlink_target_fsp will be None. + + Raises: + RootCheckError: If path attempts to escape root directory + """ + if path is None or path == "": + full_path = self.root_path + else: + full_path = os.path.join(self.root_path, path) + return self._get_file_info_from_path(full_path, current_user, session) + + + def check_is_binary(self, path: Optional[str] = None, sample_size: int = 4096) -> bool: + """ + Check if a file is likely binary by reading a sample of its contents. + + Args: + path (str): The relative path to the file to check. + May be None, in which case the root is checked (always returns False for directories). + sample_size (int): Number of bytes to read for binary detection. Defaults to 4096. + + Returns: + bool: True if the file appears to be binary, False otherwise. + Returns False for directories. Raises: ValueError: If path attempts to escape root directory + FileNotFoundError: If the file does not exist + PermissionError: If the file cannot be read """ full_path = self._check_path_in_root(path) - return self._get_file_info_from_path(full_path, current_user) + + # Directories are not binary + if os.path.isdir(full_path): + return False + + try: + with open(full_path, 'rb') as f: + sample = f.read(sample_size) + return is_likely_binary(sample) + except Exception as e: + # If we can't read the file, assume it's binary to be safe + logger.warning(f"Could not read file sample for binary detection: {e}") + return True def check_is_binary(self, path: Optional[str] = None, sample_size: int = 4096) -> bool: @@ -281,7 +475,7 @@ def check_is_binary(self, path: Optional[str] = None, sample_size: int = 4096) - return True - def yield_file_infos(self, path: Optional[str] = None, current_user: str = None) -> Generator[FileInfo, None, None]: + def yield_file_infos(self, path: Optional[str] = None, current_user: str = None, session = None) -> Generator[FileInfo, None, None]: """ Yield a FileInfo object for each child of the given path. @@ -290,6 +484,8 @@ def yield_file_infos(self, path: Optional[str] = None, current_user: str = None) May be None, in which case the root directory is listed. current_user (str): The username of the current user for permission checking. May be None, in which case hasRead and hasWrite will be None. + session: Database session for symlink resolution. + May be None, in which case symlink_target_fsp will be None for symlinks. Raises: PermissionError: If the path is not accessible due to permissions. @@ -304,9 +500,10 @@ def yield_file_infos(self, path: Optional[str] = None, current_user: str = None) for entry in entries: entry_path = os.path.join(full_path, entry) try: - yield self._get_file_info_from_path(entry_path, current_user) - except (FileNotFoundError, PermissionError, OSError) as e: - logger.error(f"Error accessing entry: {entry_path}: {e}") + yield self._get_file_info_from_path(entry_path, current_user, session) + except PermissionError as e: + # Skip files we don't have permission to access + logger.error(f"Permission denied accessing entry: {entry_path}: {e}") continue diff --git a/frontend/src/components/ui/BrowsePage/FileBrowser.tsx b/frontend/src/components/ui/BrowsePage/FileBrowser.tsx index 770c9bdd..5d720ff9 100644 --- a/frontend/src/components/ui/BrowsePage/FileBrowser.tsx +++ b/frontend/src/components/ui/BrowsePage/FileBrowser.tsx @@ -1,3 +1,4 @@ +import { useEffect } from 'react'; import type { MouseEvent } from 'react'; import { Typography } from '@material-tailwind/react'; import toast from 'react-hot-toast'; @@ -52,7 +53,8 @@ export default function FileBrowser({ fspName, fileQuery, fileBrowserState, - updateFilesWithContextMenuClick + updateFilesWithContextMenuClick, + clearSelection } = useFileBrowserContext(); const { folderPreferenceMap, handleContextMenuFavorite } = usePreferencesContext(); @@ -83,6 +85,17 @@ export default function FileBrowser({ const isN5Dir = detectN5(fileQuery.data?.files as FileOrFolder[]); + // Escape key clears row selection and reverts properties to current directory + useEffect(() => { + const handleKeyDown = (e: KeyboardEvent) => { + if (e.key === 'Escape') { + clearSelection(); + } + }; + document.addEventListener('keydown', handleKeyDown); + return () => document.removeEventListener('keydown', handleKeyDown); + }, [clearSelection]); + // Handle right-click on file - FileBrowser-specific logic const handleFileContextMenu = ( e: MouseEvent, @@ -122,7 +135,9 @@ export default function FileBrowser({ toast.error(`Error downloading file: ${result.error}`); } }, - shouldShow: !propertiesTarget.is_dir + shouldShow: + !fileBrowserState.selectedFiles[0]?.is_dir && + !fileBrowserState.selectedFiles[0]?.is_symlink }, { name: isFavorite ? 'Unset favorite' : 'Set favorite', @@ -134,14 +149,19 @@ export default function FileBrowser({ toast.success(`Favorite ${isFavorite ? 'removed!' : 'added!'}`); } }, - shouldShow: fileBrowserState.selectedFiles[0]?.is_dir ?? false + shouldShow: + fileBrowserState.selectedFiles[0]?.is_dir && + !fileBrowserState.selectedFiles[0].is_symlink }, { name: 'Convert images to OME-Zarr', action: () => { setShowConvertFileDialog(true); }, - shouldShow: tasksEnabled && propertiesTarget.is_dir + shouldShow: + tasksEnabled && + fileBrowserState.selectedFiles[0]?.is_dir && + !fileBrowserState.selectedFiles[0]?.is_symlink }, { name: 'Rename', @@ -155,7 +175,7 @@ export default function FileBrowser({ action: () => { setShowPermissionsDialog(true); }, - shouldShow: !propertiesTarget.is_dir + shouldShow: true }, { name: 'Delete', diff --git a/frontend/src/components/ui/BrowsePage/FileTable.tsx b/frontend/src/components/ui/BrowsePage/FileTable.tsx index a9dfa2a4..1ea11844 100644 --- a/frontend/src/components/ui/BrowsePage/FileTable.tsx +++ b/frontend/src/components/ui/BrowsePage/FileTable.tsx @@ -9,7 +9,7 @@ import { type SortingState } from '@tanstack/react-table'; import { IconButton, Typography } from '@material-tailwind/react'; -import { TbFile } from 'react-icons/tb'; +import { TbFile, TbLink, TbLinkOff } from 'react-icons/tb'; import { HiOutlineEllipsisHorizontalCircle, HiOutlineFolder @@ -59,8 +59,19 @@ export default function Table({ const file = row.original; const name = getValue() as string; let link = '#'; + let isBrokenSymlink = false; - if (file.is_dir && fileQuery.data?.currentFileSharePath) { + if (file.is_symlink && file.symlink_target_fsp) { + // Valid symlink with known target in a valid file share + link = makeBrowseLink( + file.symlink_target_fsp.fsp_name, + file.symlink_target_fsp.subpath + ) as string; + } else if (file.is_symlink && !file.symlink_target_fsp) { + // Broken symlink - no valid target + isBrokenSymlink = true; + } else if (file.is_dir && fileQuery.data?.currentFileSharePath) { + // Regular directory link = makeBrowseLink( fileQuery.data?.currentFileSharePath.name, file.path @@ -69,13 +80,21 @@ export default function Table({ return (
- {file.is_dir ? ( + {isBrokenSymlink ? ( + + ) : file.is_symlink ? ( + + ) : file.is_dir ? ( ) : ( )} - {file.is_dir ? ( + {isBrokenSymlink ? ( + + {name} + + ) : file.is_dir || file.is_symlink ? ( {name} @@ -94,20 +113,25 @@ export default function Table({ { id: 'actions', header: 'Actions', - cell: ({ row }) => ( -
- { - e.stopPropagation(); - handleContextMenuClick(e as any, row.original); - }} - variant="ghost" - > - - -
- ), + cell: ({ row }) => { + const file = row.original; + const isBrokenSymlink = file.is_symlink && !file.symlink_target_fsp; + return ( +
+ { + e.stopPropagation(); + handleContextMenuClick(e as any, row.original); + }} + variant="ghost" + > + + +
+ ); + }, size: 30, enableSorting: false } diff --git a/frontend/src/components/ui/BrowsePage/fileTableColumns.tsx b/frontend/src/components/ui/BrowsePage/fileTableColumns.tsx index 34327ae4..65e39e99 100644 --- a/frontend/src/components/ui/BrowsePage/fileTableColumns.tsx +++ b/frontend/src/components/ui/BrowsePage/fileTableColumns.tsx @@ -10,9 +10,14 @@ import { formatUnixTimestamp, formatFileSize } from '@/utils'; export const typeColumn: ColumnDef = { accessorKey: 'is_dir', header: 'Type', - cell: ({ getValue }) => ( - {getValue() ? 'Folder' : 'File'} - ), + cell: ({ row }) => { + const file = row.original; + if (file.is_symlink) { + // Valid symlink + return Symlink; + } + return {file.is_dir ? 'Folder' : 'File'}; + }, sortingFn: (rowA, rowB) => { const a = rowA.original.is_dir; const b = rowB.original.is_dir; diff --git a/frontend/src/components/ui/PropertiesDrawer/OverviewTable.tsx b/frontend/src/components/ui/PropertiesDrawer/OverviewTable.tsx index 22cdfba5..f7c09ef0 100644 --- a/frontend/src/components/ui/PropertiesDrawer/OverviewTable.tsx +++ b/frontend/src/components/ui/PropertiesDrawer/OverviewTable.tsx @@ -15,7 +15,15 @@ export default function OverviewTable({ Type - {file ? (file.is_dir ? 'Folder' : 'File') : null} + {file + ? file.is_symlink + ? file.symlink_target_fsp + ? 'Symlink' + : 'Symlink (broken)' + : file.is_dir + ? 'Folder' + : 'File' + : null} @@ -31,7 +39,11 @@ export default function OverviewTable({ Size - {file ? (file.is_dir ? '—' : formatFileSize(file.size)) : null} + {file + ? file.is_dir && !file.is_symlink + ? '—' + : formatFileSize(file.size) + : null} diff --git a/frontend/src/components/ui/PropertiesDrawer/PropertiesDrawer.tsx b/frontend/src/components/ui/PropertiesDrawer/PropertiesDrawer.tsx index 36dbe8f6..f14e8515 100644 --- a/frontend/src/components/ui/PropertiesDrawer/PropertiesDrawer.tsx +++ b/frontend/src/components/ui/PropertiesDrawer/PropertiesDrawer.tsx @@ -25,6 +25,7 @@ import { useTicketContext } from '@/contexts/TicketsContext'; import { useProxiedPathContext } from '@/contexts/ProxiedPathContext'; import { useExternalBucketContext } from '@/contexts/ExternalBucketContext'; import useDataToolLinks from '@/hooks/useDataToolLinks'; +import { TbLink, TbLinkOff } from 'react-icons/tb'; type PropertiesDrawerProps = { readonly togglePropertiesDrawer: () => void; @@ -38,17 +39,23 @@ type PropertiesDrawerProps = { function CopyPathButton({ path, - isDataLink + isDataLink, + isSymlink }: { readonly path: string; readonly isDataLink?: boolean; + readonly isSymlink?: boolean; }) { return (
- {isDataLink ? 'Data Link: ' : 'Path: '} + {isDataLink + ? 'Data Link: ' + : isSymlink + ? 'Linked path: ' + : 'Path: '} {path} @@ -60,11 +67,11 @@ function CopyPathButton({ const result = await copyToClipboard(path); if (result.success) { toast.success( - `${isDataLink ? 'Data link' : 'Path'} copied to clipboard!` + `${isDataLink ? 'Data link' : isSymlink ? 'Linked path' : 'Path'} copied to clipboard!` ); } else { toast.error( - `Failed to copy ${isDataLink ? 'data link' : 'path'}. Error: ${result.error}` + `Failed to copy ${isDataLink ? 'data link' : isSymlink ? 'linked path' : 'path'}. Error: ${result.error}` ); } }} @@ -137,21 +144,44 @@ export default function PropertiesDrawer({
- {fileBrowserState.propertiesTarget ? ( + {fileQuery.data?.currentFileOrFolder && + fileBrowserState.propertiesTarget ? (
- {fileBrowserState.propertiesTarget.is_dir ? ( - + {fileBrowserState.propertiesTarget.is_symlink ? ( + <> + {fileBrowserState.propertiesTarget.symlink_target_fsp ? ( + + ) : ( + + )} +
+ + + {fileBrowserState.propertiesTarget.name} + + +
+ ) : ( - + <> + {fileBrowserState.propertiesTarget.is_dir ? ( + + ) : ( + + )} + + + {fileBrowserState.propertiesTarget?.name} + + + )} - - - {fileBrowserState.propertiesTarget?.name} - -
) : ( @@ -180,7 +210,7 @@ export default function PropertiesDrawer({ Permissions - {tasksEnabled ? ( + {tasksEnabled && !fileBrowserState.propertiesTarget.is_symlink ? ( - + {fileBrowserState.propertiesTarget.is_dir && (proxiedPathByFspAndPathQuery.isPending || @@ -309,7 +342,7 @@ export default function PropertiesDrawer({ {/*Task panel*/} - {tasksEnabled ? ( + {tasksEnabled && !fileBrowserState.propertiesTarget.is_symlink ? ( void; updateFilesWithContextMenuClick: (file: FileOrFolder) => void; + clearSelection: () => void; }; const FileBrowserContext = createContext(null); @@ -104,6 +107,7 @@ export const FileBrowserContextProvider = ({ // Internal state for UI interactions const [internalState, setInternalState] = useState({ propertiesTargetPath: null, + propertiesTargetName: null, selectedFiles: [] }); @@ -133,8 +137,12 @@ export const FileBrowserContextProvider = ({ file: FileOrFolder, showFilePropertiesDrawer: boolean ) => { - // If clicking on a file (not directory), navigate to the file URL - if (!file.is_dir && fileQuery.data?.currentFileSharePath) { + // If clicking on a regular file (not directory or symlink), navigate to the file URL + if ( + !file.is_dir && + !file.is_symlink && + fileQuery.data?.currentFileSharePath + ) { const fileLink = makeBrowseLink( fileQuery.data?.currentFileSharePath.name, file.path @@ -151,15 +159,14 @@ export const FileBrowserContextProvider = ({ showFilePropertiesDrawer ? [file] : []; - const newPropertiesTargetPath = + const isSelected = currentIndex === -1 || internalState.selectedFiles.length > 1 || - showFilePropertiesDrawer - ? file.path - : null; + showFilePropertiesDrawer; updateInternalState({ - propertiesTargetPath: newPropertiesTargetPath, + propertiesTargetPath: isSelected ? file.path : null, + propertiesTargetName: isSelected ? file.name : null, selectedFiles: newSelectedFiles }); }; @@ -171,10 +178,19 @@ export const FileBrowserContextProvider = ({ updateInternalState({ propertiesTargetPath: file.path, + propertiesTargetName: file.name, selectedFiles: newSelectedFiles }); }; + const clearSelection = useCallback(() => { + setInternalState({ + propertiesTargetPath: fileQuery.data?.currentFileOrFolder?.path || null, + propertiesTargetName: null, + selectedFiles: [] + }); + }, [fileQuery.data?.currentFileOrFolder?.path]); + // Update client state when URL changes (navigation to different file/folder) // Set propertiesTarget to the current directory/file being viewed useEffect( @@ -185,6 +201,7 @@ export const FileBrowserContextProvider = ({ setInternalState({ propertiesTargetPath: fileQuery.data?.currentFileOrFolder?.path || null, + propertiesTargetName: null, selectedFiles: [] }); } @@ -208,11 +225,15 @@ export const FileBrowserContextProvider = ({ if (renameMutation.isSuccess && renameMutation.variables) { const { oldPath, newPath } = renameMutation.variables; - // If the renamed file was the propertiesTarget, update to the new path + // If the renamed file was the propertiesTarget, update to the new path/name if (internalState.propertiesTargetPath === oldPath) { + const newName = newPath.includes('/') + ? newPath.split('/').pop()! + : newPath; setInternalState(prev => ({ ...prev, - propertiesTargetPath: newPath + propertiesTargetPath: newPath, + propertiesTargetName: newName })); } // Reset mutation state to prevent re-running @@ -225,14 +246,14 @@ export const FileBrowserContextProvider = ({ renameMutation ]); - // Derive propertiesTarget from propertiesTargetPath and fresh query data + // Derive propertiesTarget from propertiesTargetPath/Name and fresh query data // This ensures mutations (rename, permissions) are correctly reflected and don't use a useEffect const propertiesTarget = useMemo(() => { if (!internalState.propertiesTargetPath || !fileQuery.data) { return null; } - // Check if propertiesTargetPath matches the current folder + // Check if propertiesTargetPath matches the current folder (navigation case) if ( fileQuery.data.currentFileOrFolder?.path === internalState.propertiesTargetPath @@ -240,20 +261,54 @@ export const FileBrowserContextProvider = ({ return fileQuery.data.currentFileOrFolder; } - // Otherwise, is it a child of current folder + // Find child by name when available (click selection case). + // Name is unique within a directory and avoids ambiguity from symlinks + // whose resolved path may collide with their target's path. + if (internalState.propertiesTargetName) { + const foundFile = fileQuery.data.files.find( + f => f.name === internalState.propertiesTargetName + ); + if (foundFile) { + return foundFile; + } + } + + // Fallback: find by path const foundFile = fileQuery.data.files.find( f => f.path === internalState.propertiesTargetPath ); return foundFile || null; - }, [internalState.propertiesTargetPath, fileQuery.data]); + }, [ + internalState.propertiesTargetPath, + internalState.propertiesTargetName, + fileQuery.data + ]); + + // Compute the effective path for data link operations. + // For symlinks, construct the unresolved path from parent + symlink name + // (propertiesTarget.path is the resolved target path, not the symlink location). + // For non-symlinks, use propertiesTarget.path directly. + const dataLinkPath = useMemo(() => { + const parentPath = fileQuery.data?.currentFileOrFolder?.path; + if (!propertiesTarget) { + return parentPath || null; + } + if (propertiesTarget.is_symlink && propertiesTarget.name && parentPath) { + return parentPath === '.' + ? propertiesTarget.name + : `${parentPath}/${propertiesTarget.name}`; + } + return propertiesTarget.path; + }, [propertiesTarget, fileQuery.data?.currentFileOrFolder?.path]); return ( {children} diff --git a/frontend/src/contexts/ProxiedPathContext.tsx b/frontend/src/contexts/ProxiedPathContext.tsx index 5d8c97b3..5d964540 100644 --- a/frontend/src/contexts/ProxiedPathContext.tsx +++ b/frontend/src/contexts/ProxiedPathContext.tsx @@ -25,6 +25,9 @@ type ProxiedPathContextType = { proxiedPathByFspAndPathQuery: ReturnType< typeof useProxiedPathByFspAndPathQuery >; + currentDirProxiedPathQuery: ReturnType< + typeof useProxiedPathByFspAndPathQuery + >; createProxiedPathMutation: ReturnType; deleteProxiedPathMutation: ReturnType; }; @@ -46,21 +49,34 @@ export const ProxiedPathProvider = ({ }: { readonly children: ReactNode; }) => { - const { fileQuery } = useFileBrowserContext(); + const { fileQuery, fileBrowserState } = useFileBrowserContext(); + + const isReady = !fileQuery.isPending && !fileQuery.isError; // Initialize all queries and mutations const allProxiedPathsQuery = useAllProxiedPathsQuery(); + + // Query for the properties target (used by the data link toggle in PropertiesDrawer) const proxiedPathByFspAndPathQuery = useProxiedPathByFspAndPathQuery( + fileQuery.data?.currentFileSharePath?.name, + fileBrowserState.dataLinkPath ?? undefined, + isReady + ); + + // Query for the current browsed directory (used by viewer icon URLs) + const currentDirProxiedPathQuery = useProxiedPathByFspAndPathQuery( fileQuery.data?.currentFileSharePath?.name, fileQuery.data?.currentFileOrFolder?.path, - !fileQuery.isPending && !fileQuery.isError + isReady ); + const createProxiedPathMutation = useCreateProxiedPathMutation(); const deleteProxiedPathMutation = useDeleteProxiedPathMutation(); const value: ProxiedPathContextType = { allProxiedPathsQuery, proxiedPathByFspAndPathQuery, + currentDirProxiedPathQuery, createProxiedPathMutation, deleteProxiedPathMutation }; diff --git a/frontend/src/hooks/useDataToolLinks.ts b/frontend/src/hooks/useDataToolLinks.ts index fe5991f1..f882c69b 100644 --- a/frontend/src/hooks/useDataToolLinks.ts +++ b/frontend/src/hooks/useDataToolLinks.ts @@ -19,7 +19,7 @@ export default function useDataToolLinks( pendingToolKey: PendingToolKey, setPendingToolKey: Dispatch> ): { - handleCreateDataLink: () => Promise; + handleCreateDataLink: (pathOverride?: string) => Promise; handleDeleteDataLink: (proxiedPath: ProxiedPath) => Promise; handleToolClick: (toolKey: PendingToolKey) => Promise; handleDialogConfirm: () => Promise; @@ -31,7 +31,7 @@ export default function useDataToolLinks( export default function useDataToolLinks( setShowDataLinkDialog: Dispatch> ): { - handleCreateDataLink: () => Promise; + handleCreateDataLink: (pathOverride?: string) => Promise; handleDeleteDataLink: (proxiedPath: ProxiedPath) => Promise; handleToolClick: (toolKey: PendingToolKey) => Promise; handleDialogConfirm: () => Promise; @@ -49,24 +49,27 @@ export default function useDataToolLinks( const currentUrlsRef = useRef(openWithToolUrls); currentUrlsRef.current = openWithToolUrls; - const { fileQuery } = useFileBrowserContext(); + const { fileQuery, fileBrowserState } = useFileBrowserContext(); const { createProxiedPathMutation, deleteProxiedPathMutation, allProxiedPathsQuery, - proxiedPathByFspAndPathQuery + currentDirProxiedPathQuery } = useProxiedPathContext(); const { areDataLinksAutomatic } = usePreferencesContext(); const { externalDataUrlQuery } = useExternalBucketContext(); const { handleCopy, showCopiedTooltip } = useCopyTooltip(); - const handleCreateDataLink = async (): Promise => { + const handleCreateDataLink = async ( + pathOverride?: string + ): Promise => { + const path = pathOverride || fileBrowserState.dataLinkPath; if (!fileQuery.data?.currentFileSharePath) { toast.error('No file share path selected'); return false; } - if (!fileQuery.data?.currentFileOrFolder) { + if (!path) { toast.error('No folder selected'); return false; } @@ -74,7 +77,7 @@ export default function useDataToolLinks( try { await createProxiedPathMutation.mutateAsync({ fsp_name: fileQuery.data.currentFileSharePath.name, - path: fileQuery.data.currentFileOrFolder.path + path }); toast.success('Data link created successfully'); await allProxiedPathsQuery.refetch(); @@ -134,7 +137,11 @@ export default function useDataToolLinks( return; } - const success = await handleCreateDataLink(); + // Viewer icon flow: always create data link for the current directory, + // not the properties target (which may be a selected subdirectory row) + const success = await handleCreateDataLink( + fileQuery.data?.currentFileOrFolder?.path + ); if (!success) { // If link creation fails, exit immediately without waiting or showing navigation error return; @@ -164,7 +171,7 @@ export default function useDataToolLinks( }; const handleToolClick = async (toolKey: PendingToolKey) => { - if (!proxiedPathByFspAndPathQuery.data && !externalDataUrlQuery.data) { + if (!currentDirProxiedPathQuery.data && !externalDataUrlQuery.data) { if (areDataLinksAutomatic) { await createLinkAndExecuteAction(toolKey); } else { @@ -172,7 +179,7 @@ export default function useDataToolLinks( setShowDataLinkDialog?.(true); } } else if ( - (proxiedPathByFspAndPathQuery.data || externalDataUrlQuery.data) && + (currentDirProxiedPathQuery.data || externalDataUrlQuery.data) && openWithToolUrls ) { await executeToolAction(toolKey, openWithToolUrls); diff --git a/frontend/src/hooks/useN5Metadata.ts b/frontend/src/hooks/useN5Metadata.ts index f432c1ce..b56bf955 100644 --- a/frontend/src/hooks/useN5Metadata.ts +++ b/frontend/src/hooks/useN5Metadata.ts @@ -49,7 +49,7 @@ function generateNeuroglancerStateForN5(dataUrl: string): string { export default function useN5Metadata() { const { fileQuery } = useFileBrowserContext(); - const { proxiedPathByFspAndPathQuery } = useProxiedPathContext(); + const { currentDirProxiedPathQuery } = useProxiedPathContext(); const { externalDataUrlQuery } = useExternalBucketContext(); // Fetch N5 metadata @@ -69,7 +69,7 @@ export default function useN5Metadata() { const neuroglancerBaseUrl = 'https://neuroglancer-demo.appspot.com/#!'; const url = - externalDataUrlQuery.data || proxiedPathByFspAndPathQuery.data?.url; + externalDataUrlQuery.data || currentDirProxiedPathQuery.data?.url; const toolUrls: N5OpenWithToolUrls = { copy: url || '', @@ -88,7 +88,7 @@ export default function useN5Metadata() { return toolUrls; }, [ metadata, - proxiedPathByFspAndPathQuery.data?.url, + currentDirProxiedPathQuery.data?.url, externalDataUrlQuery.data ]); diff --git a/frontend/src/hooks/useZarrMetadata.ts b/frontend/src/hooks/useZarrMetadata.ts index 3df57288..c0f59065 100644 --- a/frontend/src/hooks/useZarrMetadata.ts +++ b/frontend/src/hooks/useZarrMetadata.ts @@ -24,7 +24,7 @@ export type ZarrArray = zarr.Array; export default function useZarrMetadata() { const { fileQuery } = useFileBrowserContext(); - const { proxiedPathByFspAndPathQuery } = useProxiedPathContext(); + const { currentDirProxiedPathQuery } = useProxiedPathContext(); const { externalDataUrlQuery } = useExternalBucketContext(); const { disableNeuroglancerStateGeneration, @@ -97,7 +97,7 @@ export default function useZarrMetadata() { const avivatorBaseUrl = 'https://janeliascicomp.github.io/viv/'; const url = - externalDataUrlQuery.data || proxiedPathByFspAndPathQuery.data?.url; + externalDataUrlQuery.data || currentDirProxiedPathQuery.data?.url; const openWithToolUrls = { copy: url || '' } as OpenWithToolUrls; @@ -190,7 +190,7 @@ export default function useZarrMetadata() { return openWithToolUrls; }, [ metadata, - proxiedPathByFspAndPathQuery.data?.url, + currentDirProxiedPathQuery.data?.url, externalDataUrlQuery.data, disableNeuroglancerStateGeneration, useLegacyMultichannelApproach, diff --git a/frontend/src/shared.types.ts b/frontend/src/shared.types.ts index 35eaef59..7648d5f1 100644 --- a/frontend/src/shared.types.ts +++ b/frontend/src/shared.types.ts @@ -9,6 +9,11 @@ type FileOrFolder = { last_modified: number; hasRead?: boolean; hasWrite?: boolean; + is_symlink?: boolean; + symlink_target_fsp?: { + fsp_name: string; + subpath: string; + } | null; }; type FileSharePath = { diff --git a/frontend/ui-tests/tests/data-link-operations.spec.ts b/frontend/ui-tests/tests/data-link-operations.spec.ts index e81bd560..6b428d3c 100644 --- a/frontend/ui-tests/tests/data-link-operations.spec.ts +++ b/frontend/ui-tests/tests/data-link-operations.spec.ts @@ -152,4 +152,110 @@ test.describe('Data Link Operations', () => { ).toBeVisible(); }); }); + + test('Data link created for subdirectory clicked from parent shows subdirectory name', async ({ + fileglancerPage: page + }) => { + const zarrDirName = ZARR_TEST_FILE_INFO.v3_ome.dirname; + + const propertiesPanel = page + .locator('[role="complementary"]') + .filter({ hasText: 'Properties' }); + + // Click the row's Type column to select the subdirectory without navigating in + const zarrRow = page.getByRole('row').filter({ hasText: zarrDirName }); + await zarrRow.getByText('Folder', { exact: true }).click(); + + // Wait for properties panel to show the subdirectory name + await expect( + propertiesPanel.getByText(zarrDirName, { exact: true }) + ).toBeVisible({ timeout: 10000 }); + + // Wait for the data link toggle to appear + const dataLinkToggle = propertiesPanel.getByRole('checkbox', { + name: /data link/i + }); + await expect(dataLinkToggle).toBeVisible({ timeout: 10000 }); + + // Click the toggle to create a data link + await dataLinkToggle.click(); + + // Confirm in dialog + const confirmButton = page.getByRole('button', { + name: /confirm|create|yes/i + }); + await expect(confirmButton).toBeVisible({ timeout: 5000 }); + await confirmButton.click(); + + await expect( + page.getByText('Data link created successfully') + ).toBeVisible(); + + // Navigate to the Data links page + const linksNavButton = page.getByRole('link', { name: 'Data links' }); + await linksNavButton.click(); + + await expect(page.getByRole('heading', { name: /links/i })).toBeVisible(); + + // Verify the data link has the subdirectory name, not the parent directory name + const linkRow = page.getByText(zarrDirName, { exact: true }); + await expect(linkRow).toBeVisible({ timeout: 10000 }); + }); + + test('Viewer icon creates data link for current directory even when subdirectory row is selected', async ({ + fileglancerPage: page, + testDir + }) => { + const zarrDirName = ZARR_TEST_FILE_INFO.v3_ome.dirname; + + // Navigate into the zarr directory + await page.getByRole('link', { name: zarrDirName }).click(); + await expect(page.getByText('zarr.json')).toBeVisible({ timeout: 10000 }); + await expect( + page.getByRole('link', { name: 'Neuroglancer logo' }) + ).toBeVisible({ timeout: 10000 }); + + // Click on the s0 subdirectory row to select it as the properties target + const s0Row = page.getByRole('row').filter({ hasText: 's0' }); + await s0Row.getByText('Folder', { exact: true }).click(); + + // Verify s0 is selected in properties panel + const propertiesPanel = page + .locator('[role="complementary"]') + .filter({ hasText: 'Properties' }); + await expect(propertiesPanel.getByText('s0', { exact: true })).toBeVisible({ + timeout: 10000 + }); + + // Click the Neuroglancer viewer icon — this should create a data link + // for the zarr directory (currentFileOrFolder), not for s0 (propertiesTarget) + const neuroglancerLink = page.getByRole('link', { + name: 'Neuroglancer logo' + }); + await neuroglancerLink.click(); + + // Confirm in dialog + const confirmButton = page.getByRole('button', { + name: /confirm|create|yes/i + }); + await expect(confirmButton).toBeVisible({ timeout: 5000 }); + await confirmButton.click(); + + await expect( + page.getByText('Data link created successfully') + ).toBeVisible(); + + // Navigate back to check the data link on the links page + await page.goto('/browse', { waitUntil: 'domcontentloaded' }); + const linksNavButton = page.getByRole('link', { name: 'Data links' }); + await linksNavButton.click(); + + await expect(page.getByRole('heading', { name: /links/i })).toBeVisible(); + + // The data link should be for the zarr directory, not the s0 subdirectory + await expect(page.getByText(zarrDirName, { exact: true })).toBeVisible({ + timeout: 10000 + }); + await expect(page.getByText('s0', { exact: true })).not.toBeVisible(); + }); }); diff --git a/frontend/ui-tests/tests/symlink-navigation.spec.ts b/frontend/ui-tests/tests/symlink-navigation.spec.ts new file mode 100644 index 00000000..a663aea4 --- /dev/null +++ b/frontend/ui-tests/tests/symlink-navigation.spec.ts @@ -0,0 +1,358 @@ +import { expect, test as base } from '@playwright/test'; +import { join } from 'path'; +import { existsSync } from 'fs'; +import { rm, mkdir, writeFile, symlink } from 'fs/promises'; +import { navigateToScratchFsp, navigateToTestDir } from '../utils/navigation'; +import { mockAPI, teardownMockAPI } from '../mocks/api'; +import { cleanDatabase } from '../utils/db-cleanup'; +import { randomBytes } from 'crypto'; + +import type { Page } from '@playwright/test'; + +type SymlinkFixtures = { + fileglancerPage: Page; + testDir: string; +}; + +/** + * Create symlink test fixtures in the given directory. + * Creates: + * - target_subdir/ (a directory) + * - target_subdir/target_file.txt (a file) + * - symlink_to_subdir -> target_subdir (symlink to directory) + * - symlink_to_file -> target_subdir/target_file.txt (symlink to file) + * - broken_symlink -> /nonexistent/path (broken symlink) + * - regular_file.txt (a regular file for comparison) + */ +async function createSymlinkTestFixtures(testDir: string): Promise { + // Create target directory + const targetSubdir = join(testDir, 'target_subdir'); + await mkdir(targetSubdir, { recursive: true }); + + // Create target file inside target directory + await writeFile( + join(targetSubdir, 'target_file.txt'), + 'This is the target file content' + ); + + // Create symlink to directory (same share) + await symlink(targetSubdir, join(testDir, 'symlink_to_subdir')); + + // Create symlink to file + await symlink( + join(targetSubdir, 'target_file.txt'), + join(testDir, 'symlink_to_file') + ); + + // Create broken symlink - pointing to nonexistent path + await symlink( + '/nonexistent/path/that/does/not/exist', + join(testDir, 'broken_symlink') + ); + + // Create a regular file for comparison + await writeFile(join(testDir, 'regular_file.txt'), 'Regular file content'); + + console.log('[Symlink Fixture] Created symlink test fixtures in', testDir); +} + +const openFileglancer = async (page: Page) => { + await page.goto('/', { + waitUntil: 'domcontentloaded' + }); + await page.waitForSelector('text=Log In', { timeout: 10000 }); + + const loginForm = page.getByRole('textbox', { name: 'Username' }); + const loginSubmitBtn = page.getByRole('button', { name: 'Log In' }); + await loginForm.fill('testUser'); + await loginSubmitBtn.click(); + + await page.waitForSelector('text=Zones', { timeout: 10000 }); +}; + +/** + * Custom Playwright fixture for symlink tests. + * + * Similar to the main fileglancer-fixture but creates symlink test data + * instead of the standard test files and zarr directories. + */ +const test = base.extend({ + testDir: async ({}, use) => { + const scratchDir = join(global.testTempDir, 'scratch'); + const uniqueId = randomBytes(8).toString('hex'); + const testDir = `symlink-test-${uniqueId}`; + const fullPath = join(scratchDir, testDir); + + console.log( + `[Symlink Fixture] Creating unique test directory: ${fullPath}` + ); + + // Create test directory and populate with symlink fixtures + await mkdir(fullPath, { recursive: true }); + await createSymlinkTestFixtures(fullPath); + + // Provide test directory NAME (not full path) to the test + await use(testDir); + + // Cleanup test directory after test completes + console.log(`[Symlink Fixture] Cleaning up test directory: ${fullPath}`); + if (existsSync(fullPath)) { + await rm(fullPath, { recursive: true, force: true }); + } + }, + + fileglancerPage: async ({ page, testDir }, use) => { + const fullTestPath = join(global.testTempDir, 'scratch', testDir); + console.log(`[Symlink Fixture] Test temp dir: ${global.testTempDir}`); + console.log(`[Symlink Fixture] Test directory: ${fullTestPath}`); + + // Clean user-specific database tables + cleanDatabase(global.testTempDir); + + await mockAPI(page); + await openFileglancer(page); + await navigateToScratchFsp(page); + await navigateToTestDir(page, fullTestPath); + + // Provide the page to the test + await use(page); + + // Teardown + await teardownMockAPI(page); + await page.waitForTimeout(100); + } +}); + +test.describe('Symlink Navigation and Display', () => { + test.beforeEach( + 'Verify symlink test environment is loaded', + async ({ fileglancerPage: page }) => { + await expect( + page.getByText('regular_file.txt', { exact: true }) + ).toBeVisible({ timeout: 10000 }); + } + ); + + test('displays symlink icon and type', async ({ fileglancerPage: page }) => { + // Look for a symlink in the file table - symlink_to_file should exist + const symlinkRow = page + .getByRole('row') + .filter({ hasText: 'symlink_to_file' }); + await expect(symlinkRow).toBeVisible(); + + // Verify symlink icon is visible (TbLink icon renders as svg with text-primary class) + await expect(symlinkRow.locator('svg.text-primary')).toBeVisible(); + + // Verify Type column shows "Symlink" (use exact match to avoid matching filename) + await expect( + symlinkRow.getByText('Symlink', { exact: true }) + ).toBeVisible(); + }); + + test('navigates to symlink target within same share', async ({ + fileglancerPage: page + }) => { + await page.getByRole('link', { name: 'symlink_to_subdir' }).dblclick(); + const targetFileRow = page + .getByRole('row') + .filter({ hasText: 'target_file' }); + await expect(targetFileRow).toBeVisible(); + }); + + test('directory symlink displays as Symlink type, not Folder', async ({ + fileglancerPage: page + }) => { + // The symlink to a directory should still show as Symlink type, not Folder + const symlinkDirRow = page + .getByRole('row') + .filter({ hasText: 'symlink_to_subdir' }); + await expect(symlinkDirRow).toBeVisible(); + + // Should show "Symlink" type even though it points to a directory (use exact match) + await expect( + symlinkDirRow.getByText('Symlink', { exact: true }) + ).toBeVisible(); + + // Verify the regular directory shows as "Folder" + const regularDirRow = page + .getByRole('row') + .filter({ hasText: 'target_subdir' }); + await expect( + regularDirRow.getByText('Folder', { exact: true }) + ).toBeVisible(); + }); + + test('context menu works on symlinks', async ({ fileglancerPage: page }) => { + // Right-click on the symlink text in the table to open context menu + // Following the pattern from file-operations.spec.ts + await page + .getByText('symlink_to_file', { exact: true }) + .click({ button: 'right' }); + + // Verify context menu appears - wait for it to be visible + await expect(page.getByRole('menuitem', { name: /rename/i })).toBeVisible({ + timeout: 5000 + }); + await expect(page.getByRole('menuitem', { name: /delete/i })).toBeVisible(); + + // Close the menu by pressing Escape + await page.keyboard.press('Escape'); + }); + + test('broken symlinks are displayed with broken link icon', async ({ + fileglancerPage: page + }) => { + // The broken_symlink was created pointing to /nonexistent/path + // It should now appear in the file list with a broken link icon + const brokenLinkRow = page + .getByRole('row') + .filter({ hasText: 'broken_symlink' }); + + await expect(brokenLinkRow).toBeVisible(); + + // Verify broken link icon is visible (TbLinkOff with text-error class) + await expect(brokenLinkRow.locator('svg.text-error')).toBeVisible(); + + // Verify Type column shows "Symlink" + await expect( + brokenLinkRow.getByText('Symlink', { exact: true }) + ).toBeVisible(); + + // Verify the name is NOT a hyperlink (should be plain Typography) + // Get the text element and verify it's not an anchor + const nameCell = brokenLinkRow.locator('td').first(); + const linkElement = nameCell.locator('a'); + await expect(linkElement).not.toBeVisible(); + + // But the text itself should be visible + await expect(nameCell.getByText('broken_symlink')).toBeVisible(); + + // Verify valid files are still visible + await expect( + page.getByText('regular_file.txt', { exact: true }) + ).toBeVisible(); + await expect( + page.getByText('symlink_to_file', { exact: true }) + ).toBeVisible(); + }); + + test('working directory symlink properties panel shows correct content', async ({ + fileglancerPage: page + }) => { + // Verify properties panel is visible first + const propertiesPanel = page + .locator('[role="complementary"]') + .filter({ hasText: 'Properties' }); + await expect(propertiesPanel).toBeVisible(); + + // Click on the "Type" column to select without triggering the hyperlink + const symlinkRow = page + .getByRole('row') + .filter({ hasText: 'symlink_to_subdir' }); + await symlinkRow.getByText('Symlink', { exact: true }).click(); + + // Verify properties panel shows the symlink name in the header + await expect( + propertiesPanel.getByText('symlink_to_subdir', { exact: true }) + ).toBeVisible({ timeout: 10000 }); + + // Verify "Linked path:" label (symlink-specific) + await expect(propertiesPanel.getByText('Linked path:')).toBeVisible(); + + // Verify Overview table shows "Symlink" type + await expect( + propertiesPanel.locator('table').getByText('Symlink', { exact: true }) + ).toBeVisible(); + + // Verify Convert tab is not shown + await expect( + propertiesPanel.getByRole('tab', { name: 'Convert' }) + ).not.toBeVisible(); + + // Data link section should be visible (is_dir=true for directory symlink) + await expect(propertiesPanel.getByText(/data link/i).first()).toBeVisible({ + timeout: 10000 + }); + }); + + test('working file symlink properties panel shows correct content', async ({ + fileglancerPage: page + }) => { + const propertiesPanel = page + .locator('[role="complementary"]') + .filter({ hasText: 'Properties' }); + await expect(propertiesPanel).toBeVisible(); + + // Click the Type column to select + const symlinkRow = page + .getByRole('row') + .filter({ hasText: 'symlink_to_file' }); + await symlinkRow.getByText('Symlink', { exact: true }).click(); + + // Wait for properties panel to populate + await expect( + propertiesPanel.getByText('symlink_to_file', { exact: true }) + ).toBeVisible({ timeout: 10000 }); + + // Verify "Linked path:" label (symlink-specific) + await expect(propertiesPanel.getByText('Linked path:')).toBeVisible(); + + // Verify Overview table shows "Symlink" type + await expect( + propertiesPanel.locator('table').getByText('Symlink', { exact: true }) + ).toBeVisible(); + + // Verify Convert tab is not shown + await expect( + propertiesPanel.getByRole('tab', { name: 'Convert' }) + ).not.toBeVisible(); + + // Data link section should NOT be visible (is_dir=false for file symlink) + await expect( + propertiesPanel.getByText('Create data link') + ).not.toBeVisible(); + }); + + test('broken symlink properties panel shows correct content', async ({ + fileglancerPage: page + }) => { + const propertiesPanel = page + .locator('[role="complementary"]') + .filter({ hasText: 'Properties' }); + await expect(propertiesPanel).toBeVisible(); + + // Click the Type column to select + const brokenRow = page + .getByRole('row') + .filter({ hasText: 'broken_symlink' }); + await brokenRow.getByText('Symlink', { exact: true }).click(); + + // Wait for properties panel to populate + await expect( + propertiesPanel.getByText('broken_symlink', { exact: true }) + ).toBeVisible({ timeout: 10000 }); + + // Verify broken link icon (has text-error class) + await expect(propertiesPanel.locator('svg.text-error')).toBeVisible(); + + // Verify "Linked path:" label + await expect(propertiesPanel.getByText('Linked path:')).toBeVisible(); + + // Verify Overview table shows "Symlink (broken)" type + await expect( + propertiesPanel + .locator('table') + .getByText('Symlink (broken)', { exact: true }) + ).toBeVisible(); + + // Verify Convert tab is not shown + await expect( + propertiesPanel.getByRole('tab', { name: 'Convert' }) + ).not.toBeVisible(); + + // Data link section should NOT be visible (is_dir=false for broken symlink) + await expect( + propertiesPanel.getByText('Create data link') + ).not.toBeVisible(); + }); +}); diff --git a/tests/test_endpoints.py b/tests/test_endpoints.py index 179f6de3..1b3bd0f3 100644 --- a/tests/test_endpoints.py +++ b/tests/test_endpoints.py @@ -1169,6 +1169,62 @@ def test_get_content_with_symlink_no_matching_fsp(test_client, temp_dir): shutil.rmtree(external_dir) +def test_get_content_traversal_through_directory_symlink(test_client, temp_dir): + """Test that accessing a file through a directory symlink pointing outside the FSP is blocked""" + external_dir = tempfile.mkdtemp() + + try: + # Create a file in the external directory + external_file = os.path.join(external_dir, "secret.txt") + with open(external_file, "w") as f: + f.write("sensitive data") + + # Create a directory symlink in the FSP pointing to the external directory + symlink_path = os.path.join(temp_dir, "link_to_external_dir") + os.symlink(external_dir, symlink_path) + + # Try to access a file *through* the directory symlink + response = test_client.get( + "/api/content/tempdir?subpath=link_to_external_dir/secret.txt" + ) + # Should be blocked — the resolved path escapes the FSP root + assert response.status_code == 400 + + finally: + shutil.rmtree(external_dir) + + +def test_get_content_double_hop_symlink(test_client, temp_dir): + """Test that accessing a file through a chain of symlinks pointing outside the FSP is blocked""" + external_dir = tempfile.mkdtemp() + hop_dir = tempfile.mkdtemp() + + try: + # Create a file in the external directory + external_file = os.path.join(external_dir, "secret.txt") + with open(external_file, "w") as f: + f.write("sensitive data") + + # Create first hop: hop_dir/hop2 -> external_dir + hop2_path = os.path.join(hop_dir, "hop2") + os.symlink(external_dir, hop2_path) + + # Create second hop in the FSP: double_hop -> hop_dir/hop2 + symlink_path = os.path.join(temp_dir, "double_hop") + os.symlink(hop2_path, symlink_path) + + # Try to access a file through the double-hop symlink + response = test_client.get( + "/api/content/tempdir?subpath=double_hop/secret.txt" + ) + # Should be blocked — the resolved path escapes the FSP root + assert response.status_code == 400 + + finally: + shutil.rmtree(external_dir) + shutil.rmtree(hop_dir) + + def test_head_content_with_symlink(test_client, temp_dir): """Test HEAD request to /api/content endpoint with a symlink""" # Create a target file within the FSP @@ -1188,3 +1244,33 @@ def test_head_content_with_symlink(test_client, temp_dir): assert response.headers["Accept-Ranges"] == "bytes" assert "Content-Length" in response.headers assert int(response.headers["Content-Length"]) == len(target_content) + + +def test_broken_symlink_in_file_listing(test_client, temp_dir): + """Test that broken symlinks appear in /api/files response with correct properties""" + # Create a broken symlink pointing to a nonexistent path + broken_link_path = os.path.join(temp_dir, "broken_link") + os.symlink("/nonexistent/path", broken_link_path) + + # Create a regular file for comparison + regular_file = os.path.join(temp_dir, "regular.txt") + with open(regular_file, "w") as f: + f.write("content") + + # Request file listing + response = test_client.get("/api/files/tempdir") + assert response.status_code == 200 + + data = response.json() + files = data["files"] + + # Find broken symlink in response + broken_link_file = next((f for f in files if f["name"] == "broken_link"), None) + assert broken_link_file is not None, "Broken symlink should be in response" + assert broken_link_file["is_symlink"] is True, "Should be marked as symlink" + assert broken_link_file["symlink_target_fsp"] is None, "Target should be None for broken symlink" + + # Regular file should also be present + regular = next((f for f in files if f["name"] == "regular.txt"), None) + assert regular is not None, "Regular file should be in response" + assert regular["is_symlink"] is False, "Regular file should not be marked as symlink" diff --git a/tests/test_filestore.py b/tests/test_filestore.py index a7933106..f436d0f5 100644 --- a/tests/test_filestore.py +++ b/tests/test_filestore.py @@ -3,6 +3,12 @@ import pytest import tempfile import shutil + +from contextlib import contextmanager +from unittest.mock import Mock +from typing import List, Callable, Optional + +from fileglancer import database from fileglancer.filestore import Filestore, FileInfo from fileglancer.model import FileSharePath @@ -38,6 +44,36 @@ def filestore(test_dir): return Filestore(file_share_path) +@contextmanager +def mock_database_for_symlinks(file_share_paths: List[FileSharePath], + find_fsp_func: Optional[Callable] = None): + """ + Context manager to mock database functions for symlink resolution tests. + + Args: + file_share_paths: List of FileSharePath objects to return from get_file_share_paths + find_fsp_func: Optional custom function for find_fsp_from_absolute_path. + If None, the original function is preserved (not mocked). + """ + original_get_paths = database.get_file_share_paths + original_find = database.find_fsp_from_absolute_path + + def mock_get_file_share_paths(session, fsp_name=None): + paths = file_share_paths + if fsp_name: + paths = [p for p in paths if p.name == fsp_name] + return paths + + try: + database.get_file_share_paths = mock_get_file_share_paths + if find_fsp_func is not None: + database.find_fsp_from_absolute_path = find_fsp_func + yield + finally: + database.get_file_share_paths = original_get_paths + database.find_fsp_from_absolute_path = original_find + + def test_unmounted_filestore(): test_dir = "/not/a/real/path" file_share_path = FileSharePath(zone="test", name="test", mount_path=test_dir) @@ -172,3 +208,258 @@ def test_change_file_permissions_invalid_permissions(filestore): def test_change_file_permissions_invalid_path(filestore): with pytest.raises(ValueError): filestore.change_file_permissions("nonexistent.txt", "rw-r--r--") + + +# Symlink tests + +def test_symlink_detection(test_dir): + """Test that FileInfo correctly detects symlinks and their properties""" + # Create a file and a symlink to it + target_file = os.path.join(test_dir, "target.txt") + with open(target_file, "w") as f: + f.write("symlink target content") + + symlink_path = os.path.join(test_dir, "link_to_target") + os.symlink(target_file, symlink_path) + + lstat_result = os.lstat(symlink_path) + stat_result = os.stat(symlink_path) + file_info = FileInfo.from_stat("link_to_target", symlink_path, lstat_result, stat_result) + + assert file_info.is_symlink is True + assert file_info.name == "link_to_target" + + +def test_same_share_symlink_resolution_via_listing(filestore, test_dir): + """Test symlink resolution when target is within the same file share via directory listing""" + # Create target file + target_file = os.path.join(test_dir, "subdir", "target_same_share.txt") + with open(target_file, "w") as f: + f.write("same share target content") + + # Create symlink to target at root of test_dir + symlink_path = os.path.join(test_dir, "link_to_subdir_file") + os.symlink(target_file, symlink_path) + + mock_session = Mock() + fsp = FileSharePath(zone="test", name="test", mount_path=test_dir) + + def mock_find(session, path): + # Normalize paths for comparison + if os.path.realpath(path) == os.path.realpath(target_file): + return (fsp, "subdir/target_same_share.txt") + return None + + with mock_database_for_symlinks([fsp], mock_find): + # Use yield_file_infos to list directory - symlinks are detected this way + files = list(filestore.yield_file_infos("", session=mock_session)) + symlink_info = next((f for f in files if f.name == "link_to_subdir_file"), None) + + assert symlink_info is not None + assert symlink_info.is_symlink is True + assert symlink_info.symlink_target_fsp is not None + assert symlink_info.symlink_target_fsp["fsp_name"] == "test" + assert symlink_info.symlink_target_fsp["subpath"] == "subdir/target_same_share.txt" + + +def test_cross_share_symlink_resolution_via_listing(test_dir): + """Test symlink resolution when target is in a different file share via directory listing""" + # Create two file shares + share1_dir = os.path.join(test_dir, "share1") + share2_dir = os.path.join(test_dir, "share2") + os.makedirs(share1_dir) + os.makedirs(share2_dir) + + # Create target in share2 + target_file = os.path.join(share2_dir, "target.txt") + with open(target_file, "w") as f: + f.write("cross-share target") + + # Create symlink in share1 pointing to share2 + symlink_path = os.path.join(share1_dir, "link_to_share2") + os.symlink(target_file, symlink_path) + + # Create filestore for share1 + fsp1 = FileSharePath(zone="test", name="share1", mount_path=share1_dir) + fsp2 = FileSharePath(zone="test", name="share2", mount_path=share2_dir) + filestore1 = Filestore(fsp1) + + mock_session = Mock() + + def mock_find(session, path): + # Normalize paths for comparison + if os.path.realpath(path) == os.path.realpath(target_file): + return (fsp2, "target.txt") + return None + + with mock_database_for_symlinks([fsp1, fsp2], mock_find): + # Use yield_file_infos to list directory - symlinks are detected this way + files = list(filestore1.yield_file_infos("", session=mock_session)) + symlink_info = next((f for f in files if f.name == "link_to_share2"), None) + + assert symlink_info is not None + assert symlink_info.is_symlink is True + assert symlink_info.symlink_target_fsp is not None + assert symlink_info.symlink_target_fsp["fsp_name"] == "share2" + assert symlink_info.symlink_target_fsp["subpath"] == "target.txt" + + +def test_relative_symlink_resolution(test_dir): + """Test that relative symlinks are resolved correctly""" + # Create a fresh directory structure for this test + nested_dir = os.path.join(test_dir, "rel_test", "nested") + os.makedirs(nested_dir, exist_ok=True) + target_file = os.path.join(test_dir, "rel_test", "target.txt") + with open(target_file, "w") as f: + f.write("relative target") + + # Create relative symlink from nested directory pointing up + symlink_path = os.path.join(nested_dir, "link") + os.symlink("../target.txt", symlink_path) + + # Create filestore for nested_dir so symlink is listed via yield_file_infos + fsp = FileSharePath(zone="test", name="nested", mount_path=nested_dir) + fsp_rel = FileSharePath(zone="test", name="rel_test", mount_path=os.path.join(test_dir, "rel_test")) + nested_filestore = Filestore(fsp) + + mock_session = Mock() + + def mock_find(session, path): + if os.path.realpath(path) == os.path.realpath(target_file): + # Return fsp for rel_test directory + return (fsp_rel, "target.txt") + return None + + with mock_database_for_symlinks([fsp, fsp_rel], mock_find): + # List directory to find the symlink + files = list(nested_filestore.yield_file_infos("", session=mock_session)) + symlink_info = next((f for f in files if f.name == "link"), None) + + assert symlink_info is not None + assert symlink_info.is_symlink is True + assert symlink_info.symlink_target_fsp is not None + assert symlink_info.symlink_target_fsp["subpath"] == "target.txt" + + +def test_yield_file_infos_with_symlinks(filestore, test_dir): + """Test that yield_file_infos correctly lists symlinks""" + # Create file and symlink + with open(os.path.join(test_dir, "file1.txt"), "w") as f: + f.write("file 1") + + os.symlink( + os.path.join(test_dir, "file1.txt"), + os.path.join(test_dir, "link1") + ) + + mock_session = Mock() + fsp = FileSharePath(zone="test", name="test", mount_path=test_dir) + + with mock_database_for_symlinks([fsp], lambda s, p: (fsp, "file1.txt")): + files = list(filestore.yield_file_infos("", session=mock_session)) + + # Find the symlink in the list + symlink_info = next((f for f in files if f.name == "link1"), None) + assert symlink_info is not None + assert symlink_info.is_symlink is True + + +def test_broken_symlink_is_listed(filestore, test_dir): + """Test that broken symlinks are listed with is_symlink=True and symlink_target_fsp=None""" + # Create a broken symlink + broken_link = os.path.join(test_dir, "broken_link") + os.symlink("/nonexistent/path", broken_link) + + # Create a valid file for comparison + with open(os.path.join(test_dir, "valid_file.txt"), "w") as f: + f.write("valid") + + # List directory - broken symlink should now appear + files = list(filestore.yield_file_infos("")) + file_names = [f.name for f in files] + + assert "valid_file.txt" in file_names + assert "broken_link" in file_names # Broken symlink is now listed + + # Find the broken symlink and verify its properties + broken_link_info = next((f for f in files if f.name == "broken_link"), None) + assert broken_link_info is not None + assert broken_link_info.is_symlink is True + assert broken_link_info.symlink_target_fsp is None # Target not resolvable + + +def test_symlink_to_directory(filestore, test_dir): + """Test symlink pointing to a directory is detected via listing""" + # Create a directory + target_dir = os.path.join(test_dir, "target_dir") + os.makedirs(target_dir) + + # Create symlink to directory + symlink_path = os.path.join(test_dir, "link_to_dir") + os.symlink(target_dir, symlink_path) + + mock_session = Mock() + fsp = FileSharePath(zone="test", name="test", mount_path=test_dir) + + with mock_database_for_symlinks([fsp], lambda s, p: (fsp, "target_dir")): + # Use yield_file_infos to list directory - symlinks to dirs are detected this way + files = list(filestore.yield_file_infos("", session=mock_session)) + symlink_info = next((f for f in files if f.name == "link_to_dir"), None) + + assert symlink_info is not None + assert symlink_info.is_symlink is True + assert symlink_info.is_dir is True # Should also be marked as directory + assert symlink_info.symlink_target_fsp is not None + + +def test_broken_symlink_detection(test_dir, filestore): + """Test that broken symlinks are detected and returned with is_symlink=True""" + # Create a broken symlink + broken_link_path = os.path.join(test_dir, "broken_link") + os.symlink("/nonexistent/path", broken_link_path) + + # Get file infos - broken symlink should be included + file_infos = list(filestore.yield_file_infos(None)) + + # Find the broken symlink in results + broken_link_info = next((f for f in file_infos if f.name == "broken_link"), None) + + assert broken_link_info is not None, "Broken symlink should be returned" + assert broken_link_info.is_symlink is True, "Should be marked as symlink" + assert broken_link_info.symlink_target_fsp is None, "Target should be None for broken symlink" + + +def test_broken_symlink_within_share(test_dir): + """Test that broken symlinks pointing to paths within a file share don't get symlink_target_fsp populated""" + # Create a filestore + fsp = FileSharePath(zone="test", name="test_share", mount_path=test_dir) + filestore = Filestore(fsp) + + # Create a broken symlink that points to a non-existent file within the share + broken_link_path = os.path.join(test_dir, "link_to_missing_file") + missing_target = os.path.join(test_dir, "subdir", "nonexistent.txt") + os.symlink(missing_target, broken_link_path) + + mock_session = Mock() + + def mock_find(session, path): + # This would normally match the file share pattern, + # but since the target doesn't exist, we should not get here + # because os.path.exists check should return False first + normalized_path = os.path.realpath(path) + test_dir_real = os.path.realpath(test_dir) + if normalized_path.startswith(test_dir_real): + return (fsp, os.path.relpath(normalized_path, test_dir_real)) + return None + + with mock_database_for_symlinks([fsp], mock_find): + # Get file infos with session (so symlink resolution is attempted) + file_infos = list(filestore.yield_file_infos("", session=mock_session)) + + # Find the broken symlink + broken_link_info = next((f for f in file_infos if f.name == "link_to_missing_file"), None) + + # Verify the symlink is detected but target is not resolved + assert broken_link_info is not None, "Broken symlink should be listed" + assert broken_link_info.is_symlink is True, "Should be marked as symlink" + assert broken_link_info.symlink_target_fsp is None, "symlink_target_fsp should be None for broken symlink even if target path matches share pattern"