diff --git a/fileglancer/server.py b/fileglancer/server.py index 92b29d54..e8df26a9 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -134,6 +134,24 @@ def _validate_url_prefix(url_prefix: str) -> None: raise HTTPException(status_code=400, detail="Data link name must not start or end with /") if '//' in url_prefix: raise HTTPException(status_code=400, detail="Data link name must not contain consecutive slashes") + # `.` and `..` get collapsed by URL normalization at the recipient, + # which breaks key/path resolution when the link is opened. + if any(seg in (".", "..") for seg in url_prefix.split('/')): + raise HTTPException(status_code=400, detail="Data link name must not contain '.' or '..' segments") + + +def _normalize_proxied_path(path: str) -> str: + """Normalize an FSP-relative path for a proxied path record. + + The file browser surfaces the FSP root as "." (Filestore returns that as + rel_path). Strip a leading "./" and treat "." as "" so FSP-root data links + don't embed a literal "." in their share URL. + """ + if path == "." or path == "./": + return "" + if path.startswith("./"): + return path[2:] + return path def _convert_ticket(db_ticket: db.TicketDB) -> Ticket: @@ -276,6 +294,10 @@ def _resolve_proxy_info(sharing_key: str, captured_path: str) -> Tuple[dict | Re Returns (info_dict, subpath) on success, or (error_response, "") on failure. """ def try_strip_prefix(captured: str, prefix: str) -> str | None: + # Empty prefix (e.g. legacy records or FSP-root links): the entire + # captured path is the subpath. + if not prefix: + return captured if captured == prefix: return "" if captured.startswith(prefix + "/"): @@ -288,9 +310,14 @@ def try_strip_prefix(captured: str, prefix: str) -> str | None: if not proxied_path: return get_nosuchbucket_response(captured_path), "" - subpath = try_strip_prefix(captured_path, proxied_path.url_prefix) + # Treat legacy "." (FSP-root sentinel) as empty so old records that + # were created before _normalize_proxied_path still resolve. + stored_path = "" if proxied_path.path == "." else proxied_path.path + stored_prefix = "" if proxied_path.url_prefix == "." else proxied_path.url_prefix + + subpath = try_strip_prefix(captured_path, stored_prefix) if subpath is None: - subpath = try_strip_prefix(captured_path, unquote(proxied_path.url_prefix)) + subpath = try_strip_prefix(captured_path, unquote(stored_prefix)) if subpath is None: return get_error_response(404, "NoSuchKey", f"Path mismatch for sharing key {sharing_key}", captured_path), "" @@ -298,8 +325,10 @@ def try_strip_prefix(captured: str, prefix: str) -> str | None: if not fsp: return get_error_response(400, "InvalidArgument", f"File share path {proxied_path.fsp_name} not found", captured_path), "" expanded_mount_path = os.path.expanduser(fsp.mount_path) - mount_path = f"{expanded_mount_path}/{proxied_path.path}" - target_name = captured_path.rsplit('/', 1)[-1] if captured_path else os.path.basename(proxied_path.path) + # For FSP-root links (empty path) use the mount path directly to + # avoid a stray trailing slash in mount_path. + mount_path = f"{expanded_mount_path}/{stored_path}" if stored_path else expanded_mount_path + target_name = captured_path.rsplit('/', 1)[-1] if captured_path else (os.path.basename(stored_path) or fsp.name) return { "mount_path": mount_path, "target_name": target_name, @@ -980,8 +1009,17 @@ async def create_proxied_path(fsp_name: str = Query(..., description="The name o url_prefix: Optional[str] = Query(None, description="The URL path prefix after the sharing key. Defaults to basename of path."), username: str = Depends(get_current_user)): + # Normalize the FSP-relative path: the file browser surfaces the FSP + # root as "." (Filestore returns that as rel_path), but using "." in a + # share URL gets collapsed by URL normalization at the recipient, + # producing path mismatch / NoSuchBucket errors when the link is opened. + path = _normalize_proxied_path(path) + if url_prefix is None: - url_prefix = quote(os.path.basename(path), safe='/') + # basename("") is "", which would fail validation, so fall back to + # the FSP name for FSP-root links. + default_prefix = os.path.basename(path) or fsp_name + url_prefix = quote(default_prefix, safe='/') elif not _VALID_URL_PREFIX_RE.match(url_prefix): url_prefix = quote(url_prefix, safe='/') _validate_url_prefix(url_prefix) @@ -1007,6 +1045,13 @@ async def get_proxied_paths(fsp_name: str = Query(None, description="The name of path: str = Query(None, description="The path being proxied"), username: str = Depends(get_current_user)): + # The file browser surfaces the FSP root as ".", but we normalize "." + # to "" on write — apply the same normalization on lookup so the + # sidebar's "is there a link for the current folder?" query matches + # the stored row. + if path is not None: + path = _normalize_proxied_path(path) + with db.get_db_session(settings.db_url) as session: db_proxied_paths = db.get_proxied_paths(session, username, fsp_name, path) proxied_paths = [_convert_proxied_path(db_path, settings.external_proxy_url) for db_path in db_proxied_paths] @@ -1033,6 +1078,8 @@ async def update_proxied_path(sharing_key: str = Path(..., description="The shar path: Optional[str] = Query(default=None, description="The path relative to the file share path mount point"), sharing_name: Optional[str] = Query(default=None, description="The sharing path of the proxied path"), username: str = Depends(get_current_user)): + if path is not None: + path = _normalize_proxied_path(path) # If path or fsp_name is changing, validate access via worker if path is not None or fsp_name is not None: with db.get_db_session(settings.db_url) as session: diff --git a/frontend/src/__tests__/componentTests/DataLink.test.tsx b/frontend/src/__tests__/componentTests/DataLink.test.tsx index 6c7de772..6ce0519d 100644 --- a/frontend/src/__tests__/componentTests/DataLink.test.tsx +++ b/frontend/src/__tests__/componentTests/DataLink.test.tsx @@ -86,3 +86,76 @@ Could not create data link` }); }); }); + +describe('Data Link dialog at FSP root', () => { + beforeEach(async () => { + vi.clearAllMocks(); + + // Render at the FSP root (no subpath). Filestore surfaces this as ".", + // which the create flow normalizes to "" before falling back to the FSP + // name for the URL's trailing segment. + render(, { + initialEntries: ['/browse/test_fsp'] + }); + + await waitFor(() => { + expect( + screen.getByText('Are you sure you want to create a data link?') + ).toBeInTheDocument(); + }); + }); + + it('previews the FSP name as the url_prefix fallback', async () => { + const user = userEvent.setup(); + + // The preview lives inside the collapsed "Advanced settings" accordion. + await user.click(screen.getByText('Advanced settings')); + + await waitFor(() => { + expect( + screen.getByText((content: string) => + content.includes('https://...//test_fsp') + ) + ).toBeInTheDocument(); + }); + }); + + it('submits the FSP name as the url_prefix for an FSP-root link', async () => { + const { server } = await import('@/__tests__/mocks/node'); + const { http, HttpResponse } = await import('msw'); + + let capturedUrlPrefix: string | null = null; + let capturedPath: string | null = null; + server.use( + http.post('/api/proxied-path', ({ request }) => { + const url = new URL(request.url); + capturedUrlPrefix = url.searchParams.get('url_prefix'); + capturedPath = url.searchParams.get('path'); + return HttpResponse.json({ + username: 'testuser', + sharing_key: 'testkey', + sharing_name: 'test_fsp', + path: capturedPath ?? '', + fsp_name: 'test_fsp', + created_at: '2025-07-08T15:56:42.588942', + updated_at: '2025-07-08T15:56:42.588942', + url: 'http://127.0.0.1:7878/files/testkey/' + capturedUrlPrefix, + url_prefix: capturedUrlPrefix + }); + }) + ); + + const user = userEvent.setup(); + await user.click(screen.getByText('Create Data Link')); + + await waitFor(() => { + expect(toast.success).toHaveBeenCalledWith( + 'Data link created successfully' + ); + }); + // "." is normalized to "" for the backend, and the basename of "" falls + // back to the FSP name so the share URL keeps a meaningful trailing segment. + expect(capturedPath).toBe(''); + expect(capturedUrlPrefix).toBe('test_fsp'); + }); +}); diff --git a/frontend/src/components/ui/Dialogs/DataLink.tsx b/frontend/src/components/ui/Dialogs/DataLink.tsx index 13f4d4e1..62436412 100644 --- a/frontend/src/components/ui/Dialogs/DataLink.tsx +++ b/frontend/src/components/ui/Dialogs/DataLink.tsx @@ -16,6 +16,7 @@ import { makeMapKey } from '@/utils'; import { getPreferredPathForDisplay, joinPaths, + normalizeFspRootPath, normalizePosixStylePath } from '@/utils/pathHandling'; import type { FileSharePath } from '@/shared.types'; @@ -165,13 +166,24 @@ export default function DataLinkDialog(props: DataLinkDialogProps) { } const displayPath = getDisplayPath(); + // Filestore returns the FSP root as ".", but that gets collapsed by URL + // normalization at the recipient. Treat it as empty so previews and the + // submitted url_prefix fall back to the FSP name (which the create flow + // does too — see useDataToolLinks.handleCreateDataLink). + const normalizedFilePath = normalizeFspRootPath(filePath); + const fspNameForFallback = pathFsp?.name ?? ''; // Generate preview components - const folderNameOnly = filePath ? filePath.split('/').pop() || filePath : ''; + const folderNameOnly = normalizedFilePath + ? normalizedFilePath.split('/').pop() || normalizedFilePath + : fspNameForFallback; const linuxPath = pathFsp?.linux_path; - const transparentPath = - linuxPath && filePath - ? normalizePosixStylePath(joinPaths(linuxPath, filePath)) - : filePath || ''; + const transparentPath = linuxPath + ? normalizePosixStylePath( + normalizedFilePath + ? joinPaths(linuxPath, normalizedFilePath) + : linuxPath + ) + : normalizedFilePath || fspNameForFallback; // Custom subpath local state (only used in this dialog, not persisted) const [customSubpath, setCustomSubpath] = useState(folderNameOnly); diff --git a/frontend/src/hooks/useDataToolLinks.ts b/frontend/src/hooks/useDataToolLinks.ts index 50fcec5b..499d00f9 100644 --- a/frontend/src/hooks/useDataToolLinks.ts +++ b/frontend/src/hooks/useDataToolLinks.ts @@ -12,6 +12,7 @@ import { useFileBrowserContext } from '@/contexts/FileBrowserContext'; import { escapePathForUrl, joinPaths, + normalizeFspRootPath, normalizePosixStylePath } from '@/utils/pathHandling'; import useCopyTooltip from './useCopyTooltip'; @@ -35,6 +36,11 @@ export function validateUrlPrefix(value: string): string | null { if (value.includes('//')) { return 'Data link name must not contain consecutive slashes'; } + // `.` and `..` get collapsed by URL normalization at the recipient, + // which breaks key/path resolution when the link is opened. + if (value.split('/').some(seg => seg === '.' || seg === '..')) { + return "Data link name must not contain '.' or '..' segments"; + } return null; } @@ -92,15 +98,20 @@ export default function useDataToolLinks( pathOverride?: string, urlPrefixOverride?: string ): Promise => { - const path = pathOverride || fileBrowserState.dataLinkPath; + const rawPath = pathOverride ?? fileBrowserState.dataLinkPath; if (!fileQuery.data?.currentFileSharePath) { toast.error('No file share path selected'); return false; } - if (!path) { + if (rawPath === null || rawPath === undefined) { toast.error('No folder selected'); return false; } + // Filestore returns the FSP root as ".", but a literal "." in the share + // URL gets collapsed by URL normalization at the recipient, producing + // path mismatch / NoSuchBucket errors. Send "" to the backend instead. + const path = normalizeFspRootPath(rawPath); + const fspName = fileQuery.data.currentFileSharePath.name; try { let urlPrefix: string; @@ -112,16 +123,18 @@ export default function useDataToolLinks( case 'full_path': urlPrefix = escapePathForUrl( normalizePosixStylePath( - linuxPath ? joinPaths(linuxPath, path) : path + linuxPath ? joinPaths(linuxPath, path) : path || fspName ) ); break; case 'custom': - urlPrefix = path.split('/').pop() || path; + urlPrefix = path.split('/').pop() || fspName; break; case 'name': default: - urlPrefix = escapePathForUrl(path.split('/').pop() || path); + // basename of "" is "" — fall back to the FSP name for FSP-root + // links so the URL still has a meaningful trailing segment. + urlPrefix = escapePathForUrl(path.split('/').pop() || fspName); break; } } @@ -133,7 +146,7 @@ export default function useDataToolLinks( } await createProxiedPathMutation.mutateAsync({ - fsp_name: fileQuery.data.currentFileSharePath.name, + fsp_name: fspName, path, url_prefix: urlPrefix }); diff --git a/frontend/src/utils/pathHandling.ts b/frontend/src/utils/pathHandling.ts index 710c66af..f5683dd5 100644 --- a/frontend/src/utils/pathHandling.ts +++ b/frontend/src/utils/pathHandling.ts @@ -70,6 +70,21 @@ function joinPaths(...paths: string[]): string { return path.posix.join(...paths.map(path => path?.trim() ?? '')); } +/** + * Normalizes the Filestore FSP-root path (".") to an empty string. + * + * Filestore returns the FSP root as ".", but a literal "." in a share URL gets + * collapsed by URL normalization at the recipient, producing path mismatch / + * NoSuchBucket errors. Callers building data links should send "" to the + * backend instead and fall back to the FSP name for the URL's trailing segment. + * Example: + * normalizeFspRootPath('.'); // Returns '' + * normalizeFspRootPath('my_folder/my_zarr'); // Returns 'my_folder/my_zarr' + */ +function normalizeFspRootPath(filePath: string | null | undefined): string { + return !filePath || filePath === '.' ? '' : filePath; +} + /** * Constructs a sharable URL to access file contents from the browser with the Fileglancer API. * If no filePath is provided, it returns the endpoint URL with the FSP path appended - this is the base URL. @@ -266,6 +281,7 @@ export { joinPaths, makeBrowseLink, makePathSegmentArray, + normalizeFspRootPath, normalizePosixStylePath, removeLastSegmentFromPath, removeTrailingSlashes, diff --git a/tests/test_endpoints.py b/tests/test_endpoints.py index 19a43157..9a3ab00b 100644 --- a/tests/test_endpoints.py +++ b/tests/test_endpoints.py @@ -275,6 +275,104 @@ def test_create_proxied_path(test_client, temp_dir): assert "sharing_name" in data +def test_create_proxied_path_at_fsp_root(test_client, temp_dir): + """Data links created at the FSP root must produce working URLs. + + Regression for issue #368: the file browser surfaces FSP root as ".", + but a literal "." in the share URL gets collapsed by URL normalization + at the recipient, breaking key/path resolution. + """ + # Path "." (what the file browser sends at FSP root) should be normalized + response = test_client.post("/api/proxied-path?fsp_name=tempdir&path=.") + assert response.status_code == 200 + data = response.json() + assert data["path"] == "", "FSP-root path should be normalized to empty string" + assert "." not in data["url_prefix"].split("/"), \ + "url_prefix must not contain a '.' segment that browsers would collapse" + # Default url_prefix at FSP root falls back to the FSP name + assert data["url_prefix"] == "tempdir" + sharing_key = data["sharing_key"] + + # Resolving the share URL at the FSP root should not return Path mismatch. + # Create a test file at FSP root and verify it's reachable through the link. + with open(os.path.join(temp_dir, "root_file.txt"), "w") as f: + f.write("hello root") + response = test_client.get(f"/files/{sharing_key}/tempdir/root_file.txt") + assert response.status_code == 200 + assert response.text == "hello root" + + +def test_get_proxied_path_at_fsp_root_normalizes_dot(test_client): + """Sidebar's `is there a link for the current folder?` query uses path='.' + when the user is at the FSP root. After creating a link at the FSP root, + the GET endpoint must return that link for path='.' (frontend never sees + the backend normalization) — otherwise the sidebar toggle/link UI fails + to reflect the just-created link. + """ + # Create the FSP-root link via the same path the file browser sends ("."") + response = test_client.post("/api/proxied-path?fsp_name=tempdir&path=.") + assert response.status_code == 200 + created = response.json() + assert created["path"] == "" + + # Query with path="." (what the sidebar sends) — must find the link. + response = test_client.get("/api/proxied-path?fsp_name=tempdir&path=.") + assert response.status_code == 200 + paths = response.json()["paths"] + assert len(paths) == 1, f"expected sidebar query to find the FSP-root link, got {paths}" + assert paths[0]["sharing_key"] == created["sharing_key"] + + # Query with the normalized form too, for completeness + response = test_client.get("/api/proxied-path?fsp_name=tempdir&path=") + assert response.status_code == 200 + paths = response.json()["paths"] + assert len(paths) == 1 + assert paths[0]["sharing_key"] == created["sharing_key"] + + +def test_create_proxied_path_rejects_dot_url_prefix(test_client): + """url_prefix containing '.' or '..' segments must be rejected.""" + for bad_prefix in (".", "..", "./foo", "foo/.", "foo/./bar", "foo/../bar"): + response = test_client.post( + "/api/proxied-path", + params={"fsp_name": "tempdir", "path": "test_proxied_path", + "url_prefix": bad_prefix}, + ) + assert response.status_code == 400, \ + f"expected 400 for url_prefix={bad_prefix!r}, got {response.status_code}" + + +def test_resolve_legacy_dot_proxied_path(test_client, temp_dir): + """Existing DB records with stored path='.' must still resolve. + + Backward-compat for records created before the normalization fix. + """ + # Directly insert a legacy record bypassing the API normalization + with open(os.path.join(temp_dir, "legacy.txt"), "w") as f: + f.write("legacy content") + from fileglancer.database import get_db_session, ProxiedPathDB + import secrets + sharing_key = secrets.token_urlsafe(16) + # Use the same db_url the test app uses + db_url = f"sqlite:///{os.path.join(temp_dir, 'test.db')}" + with get_db_session(db_url) as session: + session.add(ProxiedPathDB( + username=TEST_USERNAME, + sharing_key=sharing_key, + sharing_name=".", + fsp_name="tempdir", + path=".", + url_prefix=".", + )) + session.commit() + + # The recipient's browser collapses the "." segment, so the request that + # actually arrives at the server has no url_prefix in the path. + response = test_client.get(f"/files/{sharing_key}/legacy.txt") + assert response.status_code == 200 + assert response.text == "legacy content" + + def test_get_proxied_paths(test_client): """Test retrieving proxied paths for a user""" path = "test_proxied_path"