From 91b2735dade4c38ca780d941c0592d5c5a56f5d0 Mon Sep 17 00:00:00 2001 From: Will Miao Date: Fri, 11 Sep 2026 22:23:24 +0800 Subject: [PATCH] fix(recipes): make batch-import directory browser work on Windows (#1106) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The browse endpoint and its frontend were written with POSIX-only assumptions, so on Windows pressing Browse immediately failed with "Access denied to this directory": - The frontend opened the browser at "/", which resolves to the current drive root on Windows. - The allowlist check used Path("/"), which has no drive letter on Windows, so relative_to() rejected every drive-qualified path — anything outside the user profile was denied. Fixes: - Empty browse path now defaults to the user home directory instead of erroring; the frontend sends "" rather than the POSIX-only "/". - The access check is platform-aware (drive-qualified on Windows, absolute on POSIX). - Parent navigation uses the server-provided parent_path; the root check is now path.parent == path (the old str/anchor comparison self-looped at Windows drive roots). - Browsing up from a Windows drive root shows a virtual list of available drives so users can switch drives without typing a path. --- py/routes/handlers/recipe_handlers.py | 92 +++++++++----- static/js/managers/BatchImportManager.js | 39 +++--- ...atchImportManager.directoryBrowser.test.js | 112 ++++++++++++++++++ .../test_batch_import_browse_directory.py | 97 +++++++++++++++ 4 files changed, 297 insertions(+), 43 deletions(-) create mode 100644 tests/frontend/managers/BatchImportManager.directoryBrowser.test.js create mode 100644 tests/routes/test_batch_import_browse_directory.py diff --git a/py/routes/handlers/recipe_handlers.py b/py/routes/handlers/recipe_handlers.py index 0a98c863..8df4eb86 100644 --- a/py/routes/handlers/recipe_handlers.py +++ b/py/routes/handlers/recipe_handlers.py @@ -3124,6 +3124,12 @@ class RecipeWorkflowHandler: class BatchImportHandler: """Handle batch import operations for recipes.""" + # Virtual path token for the Windows drive list. Browsing up from a drive + # root (e.g. C:\) lands here so users can switch drives without typing a + # path. Only meaningful on Windows; elsewhere it falls through to normal + # path handling and fails the existence check. + WINDOWS_DRIVES_TOKEN = "__drives__" + def __init__( self, *, @@ -3297,31 +3303,27 @@ class BatchImportHandler: data = await request.json() directory_path = data.get("path", "") + if os.name == "nt" and directory_path == self.WINDOWS_DRIVES_TOKEN: + return self._windows_drives_response() + + # Default to the user's home directory. The frontend previously + # sent "/" as the initial path, which is POSIX-only: on Windows it + # resolves to the current drive root and then fails the access + # check below. if not directory_path: - return web.json_response( - {"success": False, "error": "Directory path is required"}, - status=400, - ) + path = Path.home() + else: + path = Path(directory_path).expanduser().resolve() - # Normalize the path - path = Path(directory_path).expanduser().resolve() - - # Security check: ensure path is within allowed directories - # Allow common image/model directories - allowed_roots = [ - Path.home(), - Path("/"), # Allow browsing from root for flexibility - ] - - # Check if path is within any allowed root - is_allowed = False - for root in allowed_roots: - try: - path.relative_to(root) - is_allowed = True - break - except ValueError: - continue + # Access check: browsing intentionally covers the whole server + # filesystem (the server operator browses their own machine). On + # POSIX every absolute path is under "/", but Path("/") has no + # drive letter on Windows and can never anchor a drive-qualified + # path in relative_to(), so test for a drive there instead. + if os.name == "nt": + is_allowed = bool(path.drive) + else: + is_allowed = path.is_absolute() if not is_allowed: return web.json_response( @@ -3388,15 +3390,24 @@ class BatchImportHandler: directories.sort(key=lambda x: x["name"].lower()) image_files.sort(key=lambda x: x["name"].lower()) - # Add parent directory if not at root - parent_path = path.parent - show_parent = str(path) != str(path.root) + # Parent directory. A filesystem root is its own parent + # (parent == path): POSIX "/" gets no parent, while a Windows + # drive root (C:\) links up to the virtual drive list so users + # can switch drives. The previous str(path) != str(path.root) + # check misfired on Windows, where a drive root's parent is + # itself, producing an infinite self-loop. + if path.parent == path: + parent_path = ( + self.WINDOWS_DRIVES_TOKEN if os.name == "nt" else None + ) + else: + parent_path = str(path.parent) return web.json_response( { "success": True, "current_path": str(path), - "parent_path": str(parent_path) if show_parent else None, + "parent_path": parent_path, "directories": directories, "image_files": image_files, "image_count": len(image_files), @@ -3423,3 +3434,30 @@ class BatchImportHandler: except Exception as exc: self._logger.error("Error browsing directory: %s", exc, exc_info=True) return web.json_response({"success": False, "error": str(exc)}, status=500) + + def _windows_drives_response(self) -> web.Response: + """List available drive letters as a virtual directory (Windows only).""" + try: + drives = os.listdrives() + except AttributeError: # Python < 3.12 + drives = [ + f"{letter}:\\" + for letter in "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + if os.path.exists(f"{letter}:\\") + ] + directories = [ + {"name": drive, "path": drive, "is_parent": False} for drive in drives + ] + return web.json_response( + { + "success": True, + # Empty current_path marks the virtual level; the frontend + # disables folder selection there. + "current_path": "", + "parent_path": None, + "directories": directories, + "image_files": [], + "image_count": 0, + "directory_count": len(directories), + } + ) diff --git a/static/js/managers/BatchImportManager.js b/static/js/managers/BatchImportManager.js index 8a3f166c..4cebe554 100644 --- a/static/js/managers/BatchImportManager.js +++ b/static/js/managers/BatchImportManager.js @@ -18,6 +18,7 @@ export class BatchImportManager { this.results = null; this.isCancelled = false; this.isImporting = false; + this.currentParentPath = null; } /** @@ -718,9 +719,10 @@ export class BatchImportManager { browser.style.display = isVisible ? 'none' : 'block'; if (!isVisible) { - // Load initial directory when opening + // Load initial directory when opening. An empty path lets the + // server pick its default (user home); "/" would be POSIX-only. const currentPath = document.getElementById('batchDirectoryInput').value; - this.loadDirectory(currentPath || '/'); + this.loadDirectory(currentPath || ''); } } } @@ -761,6 +763,10 @@ export class BatchImportManager { const directoryCount = document.getElementById('batchDirectoryCount'); const imageCount = document.getElementById('batchImageCount'); + // Remember the server-computed parent path so the "up" navigation + // works with Windows paths too (they cannot be split on "/"). + this.currentParentPath = data.parent_path || null; + if (currentPathEl) { currentPathEl.textContent = data.current_path; } @@ -811,11 +817,9 @@ export class BatchImportManager { `; item.addEventListener('click', () => { - if (isParent) { - this.navigateToParentDirectory(); - } else { - this.loadDirectory(path); - } + // The parent entry uses the server-provided parent_path (or the + // Windows drive-list token) directly — both are plain load targets. + this.loadDirectory(path); }); return item; @@ -839,15 +843,12 @@ export class BatchImportManager { } /** - * Navigate to parent directory + * Navigate to parent directory using the path reported by the server. + * Deriving it client-side by splitting on "/" breaks Windows paths. */ navigateToParentDirectory() { - const currentPath = document.getElementById('batchCurrentPath')?.textContent; - if (currentPath) { - // Get parent path using path manipulation - const lastSeparator = currentPath.lastIndexOf('/'); - const parentPath = lastSeparator > 0 ? currentPath.substring(0, lastSeparator) : currentPath; - this.loadDirectory(parentPath); + if (this.currentParentPath) { + this.loadDirectory(this.currentParentPath); } } @@ -857,8 +858,14 @@ export class BatchImportManager { selectCurrentDirectory() { const currentPath = document.getElementById('batchCurrentPath')?.textContent; const directoryInput = document.getElementById('batchDirectoryInput'); - - if (currentPath && directoryInput) { + + if (!currentPath) { + // Virtual levels (e.g. the Windows drive list) have no path. + showToast('toast.recipes.batchImportNoDirectory', {}, 'error'); + return; + } + + if (directoryInput) { directoryInput.value = currentPath; this.toggleDirectoryBrowser(); // Close browser showToast('toast.recipes.batchImportDirectorySelected', { path: currentPath }, 'success'); diff --git a/tests/frontend/managers/BatchImportManager.directoryBrowser.test.js b/tests/frontend/managers/BatchImportManager.directoryBrowser.test.js new file mode 100644 index 00000000..0657d3f9 --- /dev/null +++ b/tests/frontend/managers/BatchImportManager.directoryBrowser.test.js @@ -0,0 +1,112 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { renderTemplate } from '../utils/domFixtures.js'; + +vi.mock('../../../static/js/utils/uiHelpers.js', () => ({ + showToast: vi.fn(), + setupAutoNewlineOnPaste: vi.fn(), +})); + +vi.mock('../../../static/js/utils/i18nHelpers.js', () => ({ + translate: (key, params = {}, fallback = null) => fallback ?? key, +})); + +vi.mock('../../../static/js/api/apiConfig.js', () => ({ + WS_ENDPOINTS: {}, +})); + +vi.mock('../../../static/js/utils/storageHelpers.js', () => ({ + getStorageItem: vi.fn(() => true), + setStorageItem: vi.fn(), +})); + +describe('BatchImportManager directory browser (#1106)', () => { + let batchImportManager; + let fetchMock; + let showToast; + + beforeEach(async () => { + vi.clearAllMocks(); + vi.resetModules(); + + document.body.innerHTML = ''; + renderTemplate('components/batch_import_modal.html'); + + fetchMock = vi.fn(async () => ({ + ok: true, + status: 200, + json: async () => ({ success: true }), + })); + vi.stubGlobal('fetch', fetchMock); + + const uiHelpers = await import('../../../static/js/utils/uiHelpers.js'); + showToast = uiHelpers.showToast; + + const batchModule = await import('../../../static/js/managers/BatchImportManager.js'); + batchImportManager = new batchModule.BatchImportManager(); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + function lastRequestBody() { + return JSON.parse(fetchMock.mock.calls.at(-1)[1].body); + } + + it('opens the browser with an empty path so the server picks the default', async () => { + // The old POSIX-only "/" initial path fails the access check on Windows. + document.getElementById('batchDirectoryInput').value = ''; + + batchImportManager.toggleDirectoryBrowser(); + + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalled()); + expect(lastRequestBody().path).toBe(''); + expect(document.getElementById('batchDirectoryBrowser').style.display).toBe('block'); + }); + + it('navigates to the parent using the server-provided path (Windows-safe)', async () => { + fetchMock.mockImplementation(async () => ({ + ok: true, + status: 200, + json: async () => ({ + success: true, + current_path: 'C:\\Users\\miao\\Pictures', + parent_path: 'C:\\Users\\miao', + directories: [], + image_files: [], + image_count: 0, + directory_count: 0, + }), + })); + + await batchImportManager.loadDirectory('C:\\Users\\miao\\Pictures'); + fetchMock.mockClear(); + + batchImportManager.navigateToParentDirectory(); + + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalled()); + expect(lastRequestBody().path).toBe('C:\\Users\\miao'); + }); + + it('refuses to select a virtual level that has no current path (drive list)', async () => { + fetchMock.mockImplementation(async () => ({ + ok: true, + status: 200, + json: async () => ({ + success: true, + current_path: '', + parent_path: null, + directories: [{ name: 'C:\\', path: 'C:\\', is_parent: false }], + image_files: [], + image_count: 0, + directory_count: 1, + }), + })); + + await batchImportManager.loadDirectory('__drives__'); + batchImportManager.selectCurrentDirectory(); + + expect(showToast).toHaveBeenCalledWith('toast.recipes.batchImportNoDirectory', {}, 'error'); + expect(document.getElementById('batchDirectoryInput').value).toBe(''); + }); +}); diff --git a/tests/routes/test_batch_import_browse_directory.py b/tests/routes/test_batch_import_browse_directory.py new file mode 100644 index 00000000..144c3da1 --- /dev/null +++ b/tests/routes/test_batch_import_browse_directory.py @@ -0,0 +1,97 @@ +import json +import logging +import os +from pathlib import Path + +import pytest + +from py.routes.handlers.recipe_handlers import BatchImportHandler + + +def _make_handler() -> BatchImportHandler: + return BatchImportHandler( + ensure_dependencies_ready=None, # browse_directory never calls it + recipe_scanner_getter=lambda: None, + civitai_client_getter=lambda: None, + logger=logging.getLogger(__name__), + batch_import_service=None, + ) + + +class _Request: + def __init__(self, path: str) -> None: + self._path = path + + async def json(self): + return {"path": self._path} + + +async def _browse(handler: BatchImportHandler, path: str): + response = await handler.browse_directory(_Request(path)) + return response, json.loads(response.text) + + +@pytest.mark.asyncio +async def test_browse_directory_lists_subdirs_and_images(tmp_path): + (tmp_path / "subdir").mkdir() + (tmp_path / "photo.png").write_bytes(b"x") + (tmp_path / "notes.txt").write_text("not an image") + + response, payload = await _browse(_make_handler(), str(tmp_path)) + + assert response.status == 200 + assert payload["success"] is True + assert payload["current_path"] == str(tmp_path) + assert [d["name"] for d in payload["directories"]] == ["subdir"] + assert [f["name"] for f in payload["image_files"]] == ["photo.png"] + assert payload["parent_path"] == str(tmp_path.parent) + + +@pytest.mark.asyncio +async def test_browse_directory_empty_path_defaults_to_home(tmp_path, monkeypatch): + # The frontend no longer sends the POSIX-only "/" as the initial path; an + # empty path must resolve to the user's home directory (#1106). + monkeypatch.setattr(Path, "home", lambda: tmp_path) + + response, payload = await _browse(_make_handler(), "") + + assert response.status == 200 + assert payload["success"] is True + assert payload["current_path"] == str(tmp_path) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX root semantics") +@pytest.mark.asyncio +async def test_browse_directory_root_has_no_parent(): + _, payload = await _browse(_make_handler(), os.path.abspath(os.sep)) + + assert payload["success"] is True + assert payload["parent_path"] is None + + +@pytest.mark.asyncio +async def test_browse_directory_windows_drives_token(monkeypatch): + # The token branch returns before any pathlib use, so faking os.name is + # enough to exercise it on POSIX (#1106). + monkeypatch.setattr(os, "name", "nt") + monkeypatch.setattr(os, "listdrives", lambda: ["C:\\", "D:\\"], raising=False) + + response, payload = await _browse( + _make_handler(), BatchImportHandler.WINDOWS_DRIVES_TOKEN + ) + + assert response.status == 200 + assert payload["success"] is True + assert payload["current_path"] == "" + assert payload["parent_path"] is None + assert [d["name"] for d in payload["directories"]] == ["C:\\", "D:\\"] + + +@pytest.mark.asyncio +async def test_browse_directory_missing_directory_returns_404(tmp_path): + response, payload = await _browse( + _make_handler(), str(tmp_path / "does-not-exist") + ) + + assert response.status == 404 + assert payload["success"] is False