Conversation
- Add is_symlink and symlink_target_fsp fields to FileInfo model - Implement symlink detection in from_stat method using lstat - Resolve symlink targets to file share paths when session is provided - Add session parameter to get_file_info, yield_file_infos, and helper methods
Update /api/files endpoint to open a database session and pass it to filestore methods, enabling symlink target resolution.
Add 7 comprehensive pytest test cases for symlink support: - Symlink detection and basic properties - Same-share symlink resolution via directory listing - Cross-share symlink resolution via directory listing - Relative symlink resolution - Symlink listing in directory (yield_file_infos) - Broken symlink handling (filtered from listings) - Symlink to directory detection All tests use mocked database sessions and verify: - is_symlink flag is correctly set - symlink_target_fsp contains correct fsp_name and subpath - Broken symlinks are skipped in directory listings - Symlinks to directories preserve is_dir=True
Add comprehensive E2E tests for symlink support: - Display symlink icon and type in file table - Navigate to symlink target within same share - Directory symlinks display as Symlink type (not Folder) - Context menu works on symlinks - Broken symlinks are not displayed - Selection shows symlink in properties panel Uses custom Playwright fixtures to create symlink test data.
…on files opens them
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…in path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
- flagged by CodeQL - Adds root_path parameter to _safe_readlink and from_stat to verify the symlink's parent directory is within the allowed root before calling os.readlink(). Uses parent directory check (not realpath) to preserve cross-share symlink support.
| if is_symlink: | ||
| try: | ||
| # Try to stat the target for file metadata | ||
| return os.stat(absolute_path) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 42 minutes ago
General approach: ensure that any path derived from user input is normalized and confirmed to reside under a known safe root before it is passed to filesystem APIs such as os.stat. Even if upstream code already enforces this, adding a small, cheap check at the point of use makes the safety invariant local and clearer to both humans and tools.
Best concrete fix here: in FileInfo.from_stat, where we currently receive absolute_path (potentially tainted) and immediately call os.lstat(absolute_path) and _get_stat_result(absolute_path, ...), add a defense-in-depth check using the root_path argument that is already passed in from Filestore._get_file_info_from_path. Specifically:
- If
root_pathis provided, computeroot_real = os.path.realpath(root_path)andabs_real = os.path.realpath(absolute_path). - Verify that
abs_realis either exactlyroot_realor starts withroot_real + os.sep. If not, log and raiseRootCheckError, just as_check_path_in_rootdoes. - Use
abs_realas the path passed intoos.lstatand_get_stat_resultto avoid inconsistencies (e.g.,/varvs/private/varon macOS), while preserving the logicalpath(relative path) used for metadata.
This keeps existing behavior intact (we only ever pass paths that are under the filestore root anyway), but makes from_stat safe even if it were ever called incorrectly and, importantly, breaks the direct tainted-data-to-os.stat flow that CodeQL is flagging.
Changes required:
- In
fileglancer/filestore.py, modifyFileInfo.from_statto:- Normalize and validate
absolute_pathagainstroot_pathwhenroot_pathis notNone, raisingRootCheckErrorotherwise. - Use the validated, normalized path (
abs_real) for bothos.lstatand_get_stat_result.
- Normalize and validate
- No changes are needed in
app.pyfor this specific alert, since it already routes throughFilestore.get_file_info, which uses_check_path_in_root.
This single change should satisfy all alert variants that point to os.stat(absolute_path) in _get_stat_result, regardless of which particular API endpoint the taint originated from.
| @@ -138,21 +138,43 @@ | ||
|
|
||
| Uses lstat to detect symlinks, then attempts to stat the target. For broken | ||
| symlinks, falls back to using lstat data. | ||
|
|
||
| Security note: | ||
| When root_path is provided, we defensively verify that absolute_path | ||
| (after realpath normalization) is still within root_path. This mirrors | ||
| Filestore._check_path_in_root and protects against any misuse of this | ||
| method with paths outside the configured root. | ||
| """ | ||
| if path is None or path == "": | ||
| raise ValueError("Path cannot be None or empty") | ||
|
|
||
| # Normalize and, when possible, validate that absolute_path stays under root_path | ||
| if root_path is not None: | ||
| root_real = os.path.realpath(root_path) | ||
| abs_real = os.path.realpath(absolute_path) | ||
|
|
||
| # Ensure the resolved path is within the resolved root | ||
| if not abs_real.startswith(root_real + os.sep) and abs_real != root_real: | ||
| # Mirror Filestore._check_path_in_root behavior | ||
| raise RootCheckError( | ||
| f"Path ({abs_real}) attempts to escape root directory ({root_real})", | ||
| abs_real, | ||
| ) | ||
| else: | ||
| # Fall back to using the provided absolute_path as-is | ||
| abs_real = absolute_path | ||
|
|
||
| # Use lstat to detect symlinks without following them | ||
| lstat_result = os.lstat(absolute_path) | ||
| lstat_result = os.lstat(abs_real) | ||
| is_symlink = stat.S_ISLNK(lstat_result.st_mode) | ||
|
|
||
| # Get the appropriate stat result (handles broken symlinks) | ||
| stat_result = cls._get_stat_result(absolute_path, is_symlink, lstat_result) | ||
| stat_result = cls._get_stat_result(abs_real, is_symlink, lstat_result) | ||
|
|
||
| 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(abs_real) | ||
| permissions = stat.filemode(stat_result.st_mode) | ||
| last_modified = stat_result.st_mtime | ||
|
|
| return lstat_result | ||
| else: | ||
| # Not a symlink, stat it normally | ||
| return os.stat(absolute_path) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Copilot Autofix
AI 43 minutes ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| raise ValueError("Path cannot be None or empty") | ||
|
|
||
| # Use lstat to detect symlinks without following them | ||
| lstat_result = os.lstat(absolute_path) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 43 minutes ago
At a high level, the fix is to (1) ensure every filesystem path derived from user input is normalized and checked to lie under the configured filestore root before being used with os.lstat, os.stat, or similar; and (2) make this invariant explicit where FileInfo.from_stat is called so both humans and static analyzers can see that absolute_path is already validated. We’ll reuse the project’s existing _check_path_in_root and realpath-based root checking rather than adding new mechanisms.
Concretely, in fileglancer/filestore.py:
-
Harden
get_absolute_path
Right now it returnsos.path.abspath(os.path.join(self.root_path, relative_path))without checking whether the resulting path stays insideroot_path. Other methods use_check_path_in_root(which performsrealpath+ prefix check). We will updateget_absolute_pathto delegate to_check_path_in_rootto ensure consistency and eliminate a potential uncontrolled-path variant that CodeQL could flag. -
Document and defensively assert safety at
from_stat
We’ll add a defensive check inFileInfo.from_statthat, whenroot_pathis provided, verifies thatabsolute_pathis insideroot_pathusing the samerealpath-based prefix strategy. This is mostly a safety assertion (we already enforce this earlier), but it makes the function self-contained from a security perspective and should address all variants where CodeQL complains aboutabsolute_pathat theos.lstatcall. If the assertion fails (meaning an internal misuse), we raiseRootCheckError, which is already imported and handled in app code.
These changes stay within the shown snippets: they only modify fileglancer/filestore.py, do not alter existing imports, and do not change behavior for valid in-root paths. For paths that somehow reach from_stat with an out-of-root absolute_path while a root_path was supplied, we’ll now fail fast with RootCheckError instead of blindly calling os.lstat on them, which is strictly safer.
| @@ -137,10 +137,28 @@ | ||
|
|
||
| Uses lstat to detect symlinks, then attempts to stat the target. For broken | ||
| symlinks, falls back to using lstat data. | ||
|
|
||
| Security note: | ||
| When root_path is provided, absolute_path is expected to have been | ||
| validated to lie within that root (for example by Filestore._check_path_in_root). | ||
| As a defense in depth, we re-check this invariant here before performing | ||
| any filesystem operations, so that user-controlled input cannot cause | ||
| os.lstat or os.stat to operate outside the intended root. | ||
| """ | ||
| if path is None or path == "": | ||
| raise ValueError("Path cannot be None or empty") | ||
|
|
||
| # Defense in depth: if we know the intended root, ensure the absolute path is inside it | ||
| if root_path is not None: | ||
| root_real = os.path.realpath(root_path) | ||
| full_real = os.path.realpath(absolute_path) | ||
| if not (full_real == root_real or full_real.startswith(root_real + os.sep)): | ||
| # Mirror the behavior of Filestore._check_path_in_root | ||
| raise RootCheckError( | ||
| f"Path ({full_real}) attempts to escape root directory ({root_real})", | ||
| full_real, | ||
| ) | ||
|
|
||
| # Use lstat to detect symlinks without following them | ||
| lstat_result = os.lstat(absolute_path) | ||
| is_symlink = stat.S_ISLNK(lstat_result.st_mode) | ||
| @@ -337,10 +351,14 @@ | ||
|
|
||
| Returns: | ||
| str: The absolute path of the Filestore. | ||
|
|
||
| Security note: | ||
| This method delegates to _check_path_in_root so that any user-supplied | ||
| relative_path cannot escape the configured root_path. The resulting | ||
| path is validated using os.path.realpath and a prefix check. | ||
| """ | ||
| if relative_path is None or relative_path == "": | ||
| return self.root_path | ||
| return os.path.abspath(os.path.join(self.root_path, relative_path)) | ||
| # Reuse the same root-checking logic used by get_file_info and others | ||
| return self._check_path_in_root(relative_path) | ||
|
|
||
|
|
||
| def get_file_info(self, path: Optional[str] = None, current_user: str = None, session = None) -> FileInfo: |
1 participant
Clickup id: 86aewngc6
This PR adds full support for symlinks in the file browser of Fileglancer, with visual distinction and navigation. Users can identify symlinks by a link icon and "Symlink" type label in the file table. Clicking on a symlink navigates to its target location within the same or different file share. Existing code filtering broken symlinks from being displayed in the file browser was kept.
To verify:
/nrs/cellmap/data/jrc_22ak351-leaf-2l. You should see the N5 directory identified as a symlink. Clicking on the name, you can navigate to the symlinked path.Changes
fileglancer/filestore.py- Addedis_symlinkandsymlink_target_fspfields to FileInfo model; implemented symlink detection usinglstat()and target resolution via existing database lookup functionfileglancer/app.py- Updated/api/filesendpoint to pass database session to filestore methods for symlink target resolutionfrontend/src/shared.types.ts- Addedis_symlinkandsymlink_target_fspfields to FileOrFolder interface -frontend/src/components/ui/BrowsePage/FileTable.tsx- Added TbLink icon for symlinks; updated link generation to navigate to symlink targetsfrontend/src/components/ui/BrowsePage/fileTableColumns.tsx- Updated type column to display "Symlink" for symlink entriesfrontend/src/components/ui/BrowsePage/FileBrowser.tsx- Removed favorite and conversion request actions from symlink context menuTests
tests/test_filestore.pycovering symlink detection, same-share/cross-share resolution, relative symlinks, broken symlink handling, and symlinks to directoriesfrontend/ui-tests/tests/symlink-navigation.spec.tscovering icon display, navigation, type labeling, context menu, broken symlinks, and properties panel selection@krokicki