From 53e3b59f4f0b0ef0f24aee59ca2e946fbb5b06d1 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Tue, 13 Oct 2026 11:37:40 +0800 Subject: [PATCH] fix(remote): allow directory symlinks in workspace picker --- .../services/services-integrations/AGENTS.md | 6 + .../src/remote_ssh/manager.rs | 85 ++++++++++- .../manager/tests/workspace_sftp.rs | 68 +++++++++ .../src/remote_ssh/remote_fs.rs | 12 +- .../ssh-remote/RemoteFileBrowser.test.tsx | 137 +++++++++++++++++- .../features/ssh-remote/RemoteFileBrowser.tsx | 48 ++++-- 6 files changed, 330 insertions(+), 26 deletions(-) diff --git a/src/crates/services/services-integrations/AGENTS.md b/src/crates/services/services-integrations/AGENTS.md index 773a5941be..3c566776a7 100644 --- a/src/crates/services/services-integrations/AGENTS.md +++ b/src/crates/services/services-integrations/AGENTS.md @@ -171,6 +171,12 @@ advertise a small handle limit and are included in the existing CI the same command with the filter ending in `workspace_sftp::openssh_real_files_over_loopback_ssh -- --ignored`. +For directory symlink browsing and workspace-path validation, use the same +package/features with `--lib directory_symlink_browsing -- --include-ignored` +and `OPENBITFUN_TEST_SFTP_SERVER` set. This runs real OpenSSH SFTP over loopback +and the Docker command adapter against a local POSIX shell fixture; the latter +does not establish real Docker-daemon or Windows WSL coverage. + For the remote Flashgrep distribution gate and retained protocol helpers, use: ```bash diff --git a/src/crates/services/services-integrations/src/remote_ssh/manager.rs b/src/crates/services/services-integrations/src/remote_ssh/manager.rs index 4191053529..3d02623a51 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/manager.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/manager.rs @@ -384,21 +384,23 @@ fn container_read_dir_script(path: &str, max_entries: Option) -> String { let entry_loop = crate::remote_ssh::shell::quote_arg(&format!( "for item do [ -e \"$item\" ] || [ -L \"$item\" ] || continue; {CONTAINER_ENTRY_METADATA_SCRIPT} done" )); + // Follow only the explicitly requested directory, like SFTP OPENDIR. + // Child links retain their own metadata and are never traversed by this list. let (head_probe, producer) = match max_entries { Some(max_entries) => ( "command -v head >/dev/null 2>&1 && head -z -n 0 /dev/null 2>&1 || { echo 'NUL-delimited bounded directory reads require head -z' >&2; exit 78; }; ", format!( - "find \"$dir\" -mindepth 1 -maxdepth 1 -print0 | head -z -n {max_entries}" + "find -H \"$dir\" -mindepth 1 -maxdepth 1 -print0 | head -z -n {max_entries}" ), ), None => ( "", - "find \"$dir\" -mindepth 1 -maxdepth 1 -print0".to_string(), + "find -H \"$dir\" -mindepth 1 -maxdepth 1 -print0".to_string(), ), }; format!( "dir={quoted}; \ - [ -d \"$dir\" ] && [ ! -L \"$dir\" ] || {{ echo 'Container directory is missing or is a symbolic link' >&2; exit 44; }}; \ + [ -d \"$dir\" ] || {{ echo 'Container path is not a directory or is inaccessible' >&2; exit 44; }}; \ command -v find >/dev/null 2>&1 && command -v xargs >/dev/null 2>&1 || {{ echo 'Container directory reads require find and xargs' >&2; exit 78; }}; \ {head_probe}\ {producer} | xargs -0 -n 64 sh -c {entry_loop} sh" @@ -7738,6 +7740,81 @@ mod tests { (manager, provider) } + #[cfg(unix)] + #[tokio::test] + async fn workspace_directory_symlink_browsing_shell() { + use std::os::unix::fs::symlink; + let temp = tempfile::tempdir().unwrap(); + let (manager, _provider) = shell_workspace_provider_fixture(temp.path()).await; + let service = crate::remote_ssh::RemoteFileService::new(Arc::new( + tokio::sync::RwLock::new(Some(manager)), + )); + let connection = "workspace-shell-fixture"; + let directory = temp.path().join("target directory"); + std::fs::create_dir(&directory).unwrap(); + std::fs::write(directory.join("sentinel.txt"), b"keep").unwrap(); + symlink("..", directory.join("parent-link")).unwrap(); + symlink(&directory, temp.path().join("absolute-link")).unwrap(); + symlink("target directory", temp.path().join("relative-link")).unwrap(); + symlink("relative-link", temp.path().join("chained-link")).unwrap(); + symlink("missing", temp.path().join("broken-link")).unwrap(); + symlink( + "target directory/sentinel.txt", + temp.path().join("file-link"), + ) + .unwrap(); + symlink("loop-link", temp.path().join("loop-link")).unwrap(); + + let entries = service + .read_dir(connection, temp.path().to_str().unwrap()) + .await + .unwrap(); + for name in ["absolute-link", "relative-link", "chained-link"] { + let entry = entries.iter().find(|entry| entry.name == name).unwrap(); + assert!(entry.is_symlink); + assert!(!entry.is_dir, "listing must retain no-follow metadata"); + assert!(service.is_dir(connection, &entry.path).await.unwrap()); + let children = service.read_dir(connection, &entry.path).await.unwrap(); + assert_eq!(children.len(), 2); + let file = children + .iter() + .find(|child| child.name == "sentinel.txt") + .unwrap(); + assert_eq!(file.path, format!("{}/sentinel.txt", entry.path)); + let link = children + .iter() + .find(|child| child.name == "parent-link") + .unwrap(); + assert!(link.is_symlink && !link.is_dir); + } + for name in ["file-link", "broken-link"] { + let path = temp.path().join(name); + assert!(!service + .is_dir(connection, path.to_str().unwrap()) + .await + .unwrap()); + assert!(service + .read_dir(connection, path.to_str().unwrap()) + .await + .is_err()); + } + assert!(service + .is_dir(connection, temp.path().join("loop-link").to_str().unwrap()) + .await + .is_err()); + service + .remove_dir_all( + connection, + temp.path().join("relative-link").to_str().unwrap(), + ) + .await + .unwrap(); + assert_eq!( + std::fs::read(directory.join("sentinel.txt")).unwrap(), + b"keep" + ); + } + #[cfg(unix)] #[tokio::test] async fn workspace_container_metadata_preserves_errors_and_follows_links() { @@ -8435,7 +8512,7 @@ mod tests { let bounded = container_read_dir_script("/workspace", Some(4096)); let unbounded = container_read_dir_script("/workspace", None); - assert!(bounded.contains("find \"$dir\" -mindepth 1 -maxdepth 1 -print0")); + assert!(bounded.contains("find -H \"$dir\" -mindepth 1 -maxdepth 1 -print0")); assert!(bounded.contains("head -z -n 4096")); assert!(bounded.contains("xargs -0 -n 64")); assert!(!bounded.contains("\"$dir\"/*")); diff --git a/src/crates/services/services-integrations/src/remote_ssh/manager/tests/workspace_sftp.rs b/src/crates/services/services-integrations/src/remote_ssh/manager/tests/workspace_sftp.rs index 8021a04f49..478faba8b8 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/manager/tests/workspace_sftp.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/manager/tests/workspace_sftp.rs @@ -765,6 +765,74 @@ async fn openssh_real_files_over_loopback_ssh() { assert_eq!(f.state.channels.load(Ordering::SeqCst), 2); } +#[cfg(unix)] +#[tokio::test] +#[ignore = "Set OPENBITFUN_TEST_SFTP_SERVER to the OpenSSH sftp-server executable"] +async fn openssh_directory_symlink_browsing() { + use std::os::unix::fs::symlink; + let executable = std::env::var("OPENBITFUN_TEST_SFTP_SERVER") + .expect("OpenSSH sftp-server executable required"); + let f = Fixture::with_openssh(Some(executable)).await; + let service = crate::remote_ssh::RemoteFileService::new(Arc::new(tokio::sync::RwLock::new( + Some(f.manager.as_ref().clone()), + ))); + let directory = f._dir.path().join("target directory"); + std::fs::create_dir(&directory).unwrap(); + std::fs::write(directory.join("sentinel.txt"), b"keep").unwrap(); + symlink("..", directory.join("parent-link")).unwrap(); + symlink(&directory, f._dir.path().join("absolute-link")).unwrap(); + symlink("target directory", f._dir.path().join("relative-link")).unwrap(); + symlink("relative-link", f._dir.path().join("chained-link")).unwrap(); + symlink("missing", f._dir.path().join("broken-link")).unwrap(); + symlink( + "target directory/sentinel.txt", + f._dir.path().join("file-link"), + ) + .unwrap(); + symlink("loop-link", f._dir.path().join("loop-link")).unwrap(); + + let entries = service + .read_dir("sftp-test", f._dir.path().to_str().unwrap()) + .await + .unwrap(); + for name in ["absolute-link", "relative-link", "chained-link"] { + let entry = entries.iter().find(|entry| entry.name == name).unwrap(); + assert!(entry.is_symlink); + assert!(!entry.is_dir, "listing must retain no-follow metadata"); + assert!(service.is_dir("sftp-test", &entry.path).await.unwrap()); + let children = service.read_dir("sftp-test", &entry.path).await.unwrap(); + assert_eq!(children.len(), 2); + let file = children + .iter() + .find(|child| child.name == "sentinel.txt") + .unwrap(); + assert_eq!(file.path, format!("{}/sentinel.txt", entry.path)); + let link = children + .iter() + .find(|child| child.name == "parent-link") + .unwrap(); + assert!(link.is_symlink && !link.is_dir); + } + for name in ["file-link", "broken-link", "loop-link"] { + let path = f._dir.path().join(name); + assert!(service + .read_dir("sftp-test", path.to_str().unwrap()) + .await + .is_err()); + } + service + .remove_dir_all( + "sftp-test", + f._dir.path().join("relative-link").to_str().unwrap(), + ) + .await + .unwrap(); + assert_eq!( + std::fs::read(directory.join("sentinel.txt")).unwrap(), + b"keep" + ); +} + #[tokio::test] async fn cancellation_during_close_finishes_the_existing_close_once() { let f = Fixture::new().await; diff --git a/src/crates/services/services-integrations/src/remote_ssh/remote_fs.rs b/src/crates/services/services-integrations/src/remote_ssh/remote_fs.rs index 33d8a6f21d..32731e391c 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/remote_fs.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/remote_fs.rs @@ -357,12 +357,14 @@ impl RemoteFileService { } } - /// Check if a remote path is a directory + /// Check if a remote path resolves to a directory, including shell workspaces. pub async fn is_dir(&self, connection_id: &str, path: &str) -> anyhow::Result { - match self.stat(connection_id, path).await? { - Some(entry) => Ok(entry.is_dir), - None => Ok(false), - } + Ok(self + .workspace_metadata(connection_id, path, true) + .await? + .is_some_and(|metadata| { + metadata.kind == openbitfun_runtime_ports::WorkspacePathKind::Directory + })) } /// Read directory contents via SFTP diff --git a/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.test.tsx b/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.test.tsx index 71f95ed437..b2c3b3b1ef 100644 --- a/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.test.tsx +++ b/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.test.tsx @@ -2,13 +2,14 @@ import React, { act } from 'react'; import { createRoot } from 'react-dom/client'; import { Simulate } from 'react-dom/test-utils'; -import { expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { RemoteFileBrowser } from './RemoteFileBrowser'; +import type { RemoteFileEntry } from './types'; globalThis.IS_REACT_ACT_ENVIRONMENT = true; const api = vi.hoisted(() => ({ - readDir: vi.fn(async () => [{ name: 'old.txt', path: '/srv/project/old.txt', isDir: false }]), + readDir: vi.fn<(connectionId: string, path: string) => Promise>(), rename: vi.fn(async () => undefined), downloadToLocalPath: vi.fn(), cancelTransfer: vi.fn(async () => undefined), @@ -17,6 +18,18 @@ vi.mock('./sshApi', () => ({ sshApi: api })); vi.mock('@/infrastructure/i18n', () => ({ useI18n: () => ({ t: (key: string) => key }) })); vi.mock('@tauri-apps/plugin-dialog', () => ({ save: vi.fn(async () => '/downloads/old.txt') })); +const fileEntry: RemoteFileEntry = { + name: 'old.txt', path: '/srv/project/old.txt', isDir: false, isFile: true, isSymlink: false, +}; +const linkEntry: RemoteFileEntry = { + name: 'linked project', path: '/srv/project/linked project', isDir: false, isFile: false, isSymlink: true, +}; + +beforeEach(() => { + vi.clearAllMocks(); + api.readDir.mockReset().mockResolvedValue([fileEntry]); +}); + it('renames a remote file once and keeps IME and outside clicks from dismissing the dialog', async () => { const container = document.createElement('div'); document.body.appendChild(container); @@ -83,3 +96,123 @@ it('stops a browser download using the id of its in-flight command', async () => delete (window as Window & { __TAURI__?: unknown }).__TAURI__; } }); + +describe('remote directory link navigation', () => { + let container: HTMLDivElement; + let root: ReturnType; + const onSelect = vi.fn(); + const row = (name: string) => [...document.querySelectorAll('.remote-file-browser__row')] + .find(node => node.querySelector('.remote-file-browser__name')?.textContent === name)!; + const openButton = () => [...document.querySelectorAll('button')] + .find(node => node.textContent === 'ssh.remote.openWorkspace')!; + const currentPath = () => document.querySelector('.remote-file-browser__footer-hint')?.textContent; + const render = async (connectionId = 'ssh-host', initialPath = '/srv/project') => { + await act(async () => root.render( + undefined} />, + )); + }; + + beforeEach(() => { + container = document.createElement('div'); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + }); + + it.each(['click', 'double click', 'context menu'])('opens a directory symlink through %s and preserves its POSIX path', async (action) => { + api.readDir.mockResolvedValueOnce([fileEntry, linkEntry]).mockResolvedValueOnce([]); + await render(); + expect(row(linkEntry.name).querySelector('.remote-file-browser__entry-icon--link')).not.toBeNull(); + await act(async () => { + if (action === 'click') row(linkEntry.name).click(); + else if (action === 'double click') Simulate.doubleClick(row(linkEntry.name)); + else row(linkEntry.name).dispatchEvent(new MouseEvent('contextmenu', { bubbles: true })); + }); + if (action === 'context menu') { + const open = [...document.querySelectorAll('[role="menuitem"]')] + .find(node => node.textContent === 'actions.open')!; + await act(async () => open.click()); + } + expect(api.readDir).toHaveBeenNthCalledWith(2, 'ssh-host', linkEntry.path); + expect(api.readDir).toHaveBeenCalledTimes(2); + expect(currentPath()).toBe(linkEntry.path); + expect(openButton().disabled).toBe(false); + act(() => openButton().click()); + expect(onSelect).toHaveBeenCalledExactlyOnceWith(linkEntry.path); + + await act(async () => row('..').click()); + expect(api.readDir).toHaveBeenLastCalledWith('ssh-host', '/srv/project'); + expect(currentPath()).toBe('/srv/project'); + }); + + it.each([ + ['file link', new Error('Not a directory')], + ['broken link', 'No such file'], + ['inaccessible link', new Error('Permission denied')], + ['cyclic link', new Error('Too many levels of symbolic links')], + ])('keeps the parent listing and error for a %s, and retries the same remote path', async (_kind, error) => { + api.readDir.mockResolvedValueOnce([linkEntry]).mockRejectedValueOnce(error).mockRejectedValueOnce(error); + await render(); + await act(async () => row(linkEntry.name).click()); + expect(openButton().disabled).toBe(true); + await act(async () => { await new Promise(resolve => setTimeout(resolve, 300)); }); + expect(currentPath()).toBe('/srv/project'); + expect(row(linkEntry.name)).toBeDefined(); + expect(document.querySelector('.remote-file-browser__error')?.textContent) + .toContain(error instanceof Error ? error.message : error); + expect(document.querySelector('.remote-file-browser__empty')).toBeNull(); + expect(onSelect).not.toHaveBeenCalled(); + + api.readDir.mockResolvedValueOnce([]); + await act(async () => document.querySelector('[aria-label="actions.retry"]')!.click()); + expect(api.readDir).toHaveBeenLastCalledWith('ssh-host', linkEntry.path); + expect(currentPath()).toBe(linkEntry.path); + }); + + it('does not select an unvalidated initial directory or call a failed load empty', async () => { + api.readDir.mockRejectedValue('Permission denied'); + await render(); + expect(openButton().disabled).toBe(true); + await act(async () => { await new Promise(resolve => setTimeout(resolve, 300)); }); + expect(openButton().disabled).toBe(true); + expect(document.querySelector('.remote-file-browser__empty')).toBeNull(); + act(() => openButton().click()); + expect(onSelect).not.toHaveBeenCalled(); + }); + + it('ignores a late link response after navigating home', async () => { + let resolveLink!: (entries: RemoteFileEntry[]) => void; + api.readDir.mockResolvedValueOnce([linkEntry]) + .mockImplementationOnce(() => new Promise(resolve => { resolveLink = resolve; })) + .mockResolvedValueOnce([fileEntry]); + await render(); + await act(async () => row(linkEntry.name).click()); + expect(openButton().disabled).toBe(true); + act(() => openButton().click()); + expect(onSelect).not.toHaveBeenCalled(); + await act(async () => document.querySelector('[aria-label="ssh.remote.homeFolder"]')!.click()); + await act(async () => resolveLink([])); + expect(currentPath()).toBe('/srv/project'); + expect(row(fileEntry.name)).toBeDefined(); + act(() => openButton().click()); + expect(onSelect).toHaveBeenCalledExactlyOnceWith('/srv/project'); + }); + + it('discards an old connection response when the host changes', async () => { + let resolveLink!: (entries: RemoteFileEntry[]) => void; + api.readDir.mockResolvedValueOnce([linkEntry]) + .mockImplementationOnce(() => new Promise(resolve => { resolveLink = resolve; })) + .mockResolvedValueOnce([fileEntry]); + await render(); + await act(async () => row(linkEntry.name).click()); + await render('other-host'); + await act(async () => resolveLink([])); + expect(api.readDir).toHaveBeenLastCalledWith('other-host', '/srv/project'); + expect(currentPath()).toBe('/srv/project'); + expect(row(fileEntry.name)).toBeDefined(); + }); +}); diff --git a/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.tsx b/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.tsx index 2608866404..b02374a8fa 100644 --- a/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.tsx +++ b/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.tsx @@ -94,6 +94,9 @@ export const RemoteFileBrowser: React.FC = ({ const textInputCompositionActiveRef = useRef(false); const [entries, setEntries] = useState([]); const [loading, setLoading] = useState(false); + const [loadedConnectionId, setLoadedConnectionId] = useState(null); + const directoryRequestRef = useRef(0); + const requestedPathRef = useRef(initialPath); const [error, setError] = useState(null); const [selectedPath, setSelectedPath] = useState(null); const [contextMenu, setContextMenu] = useState({ @@ -152,8 +155,12 @@ export const RemoteFileBrowser: React.FC = ({ // already-in-flight request still fails. Retrying once gives the recovery // path a chance to succeed before surfacing an error to the user. const loadDirectory = useCallback(async (path: string) => { + const request = ++directoryRequestRef.current; + requestedPathRef.current = path; setLoading(true); setError(null); + setSelectedPath(null); + setIsEditingPath(false); const fetchOnce = () => sshApi.readDir(connectionId, path); try { let result; @@ -163,28 +170,41 @@ export const RemoteFileBrowser: React.FC = ({ // Brief pause lets the backend complete its reconnect handshake before // we hammer it again. await new Promise((resolve) => setTimeout(resolve, 250)); + if (request !== directoryRequestRef.current) return; try { result = await fetchOnce(); } catch { throw firstErr; } } + if (request !== directoryRequestRef.current) return; result.sort((a, b) => { if (a.isDir !== b.isDir) return a.isDir ? -1 : 1; return a.name.localeCompare(b.name); }); setEntries(result); + // READDIR follows directory links on the remote host. Commit navigation + // only after it succeeds, retaining the link path and last usable listing + // when a link is broken, points to a file, or cannot be accessed. + setCurrentPath(path); + setPathInputValue(path); + setLoadedConnectionId(connectionId); } catch (e) { - setError(e instanceof Error ? e.message : 'Failed to load directory'); - setEntries([]); + if (request !== directoryRequestRef.current) return; + setError(e instanceof Error ? e.message : typeof e === 'string' ? e : 'Failed to load directory'); } finally { - setLoading(false); + if (request === directoryRequestRef.current) setLoading(false); } }, [connectionId]); useEffect(() => { - loadDirectory(currentPath); - }, [currentPath, loadDirectory]); + setEntries([]); + setLoadedConnectionId(null); + setCurrentPath(initialPath); + setPathInputValue(initialPath); + void loadDirectory(initialPath); + return () => { directoryRequestRef.current += 1; }; + }, [initialPath, loadDirectory]); // Close context menu when clicking outside useEffect(() => { @@ -198,10 +218,7 @@ export const RemoteFileBrowser: React.FC = ({ }, []); const navigateTo = (path: string) => { - setCurrentPath(path); - setPathInputValue(path); - setSelectedPath(null); - setIsEditingPath(false); + void loadDirectory(path); }; const handlePathInputKeyDown = (e: React.KeyboardEvent) => { @@ -234,7 +251,7 @@ export const RemoteFileBrowser: React.FC = ({ }; const handleEntryClick = (entry: RemoteFileEntry) => { - if (entry.isDir) { + if (entry.isDir || (selectDirectoriesOnly && entry.isSymlink)) { navigateTo(entry.path); } else if (!selectDirectoriesOnly) { setSelectedPath(entry.path); @@ -243,7 +260,7 @@ export const RemoteFileBrowser: React.FC = ({ }; const handleEntryDoubleClick = (entry: RemoteFileEntry) => { - if (entry.isDir) { + if (entry.isDir || entry.isSymlink) { navigateTo(entry.path); } }; @@ -299,7 +316,7 @@ export const RemoteFileBrowser: React.FC = ({ setDeleteConfirm({ show: true, entry }); break; case 'open': - if (entry.isDir) { + if (entry.isDir || entry.isSymlink) { navigateTo(entry.path); } else if (!selectDirectoriesOnly) { onSelect(entry.path); @@ -395,6 +412,7 @@ export const RemoteFileBrowser: React.FC = ({ }; const openSelectedWorkspace = () => { + if (loading || loadedConnectionId !== connectionId) return; onSelect(selectDirectoriesOnly ? currentPath : (selectedPath || currentPath)); }; @@ -579,7 +597,7 @@ export const RemoteFileBrowser: React.FC = ({ {error} loadDirectory(currentPath)} + onClick={() => loadDirectory(requestedPathRef.current)} title={t('actions.retry') || 'Retry'} style={{ marginLeft: 'auto', marginRight: 8 }} aria-label={t('actions.retry')} @@ -653,7 +671,7 @@ export const RemoteFileBrowser: React.FC = ({ ))} - {entries.length === 0 && !loading && ( + {entries.length === 0 && !loading && !error && loadedConnectionId === connectionId && ( {t('ssh.remote.emptyDirectory')} @@ -801,7 +819,7 @@ export const RemoteFileBrowser: React.FC = ({ variant="primary" size="sm" onClick={openSelectedWorkspace} - disabled={false} + disabled={loading || loadedConnectionId !== connectionId} > {t('ssh.remote.openWorkspace')}