mirror of
https://github.com/willmiao/ComfyUI-Lora-Manager.git
synced 2026-09-21 11:11:26 -03:00
fix(recipes): make batch-import directory browser work on Windows (#1106)
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.
This commit is contained in:
@@ -3124,6 +3124,12 @@ class RecipeWorkflowHandler:
|
|||||||
class BatchImportHandler:
|
class BatchImportHandler:
|
||||||
"""Handle batch import operations for recipes."""
|
"""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__(
|
def __init__(
|
||||||
self,
|
self,
|
||||||
*,
|
*,
|
||||||
@@ -3297,31 +3303,27 @@ class BatchImportHandler:
|
|||||||
data = await request.json()
|
data = await request.json()
|
||||||
directory_path = data.get("path", "")
|
directory_path = data.get("path", "")
|
||||||
|
|
||||||
if not directory_path:
|
if os.name == "nt" and directory_path == self.WINDOWS_DRIVES_TOKEN:
|
||||||
return web.json_response(
|
return self._windows_drives_response()
|
||||||
{"success": False, "error": "Directory path is required"},
|
|
||||||
status=400,
|
|
||||||
)
|
|
||||||
|
|
||||||
# Normalize the path
|
# 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:
|
||||||
|
path = Path.home()
|
||||||
|
else:
|
||||||
path = Path(directory_path).expanduser().resolve()
|
path = Path(directory_path).expanduser().resolve()
|
||||||
|
|
||||||
# Security check: ensure path is within allowed directories
|
# Access check: browsing intentionally covers the whole server
|
||||||
# Allow common image/model directories
|
# filesystem (the server operator browses their own machine). On
|
||||||
allowed_roots = [
|
# POSIX every absolute path is under "/", but Path("/") has no
|
||||||
Path.home(),
|
# drive letter on Windows and can never anchor a drive-qualified
|
||||||
Path("/"), # Allow browsing from root for flexibility
|
# path in relative_to(), so test for a drive there instead.
|
||||||
]
|
if os.name == "nt":
|
||||||
|
is_allowed = bool(path.drive)
|
||||||
# Check if path is within any allowed root
|
else:
|
||||||
is_allowed = False
|
is_allowed = path.is_absolute()
|
||||||
for root in allowed_roots:
|
|
||||||
try:
|
|
||||||
path.relative_to(root)
|
|
||||||
is_allowed = True
|
|
||||||
break
|
|
||||||
except ValueError:
|
|
||||||
continue
|
|
||||||
|
|
||||||
if not is_allowed:
|
if not is_allowed:
|
||||||
return web.json_response(
|
return web.json_response(
|
||||||
@@ -3388,15 +3390,24 @@ class BatchImportHandler:
|
|||||||
directories.sort(key=lambda x: x["name"].lower())
|
directories.sort(key=lambda x: x["name"].lower())
|
||||||
image_files.sort(key=lambda x: x["name"].lower())
|
image_files.sort(key=lambda x: x["name"].lower())
|
||||||
|
|
||||||
# Add parent directory if not at root
|
# Parent directory. A filesystem root is its own parent
|
||||||
parent_path = path.parent
|
# (parent == path): POSIX "/" gets no parent, while a Windows
|
||||||
show_parent = str(path) != str(path.root)
|
# 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(
|
return web.json_response(
|
||||||
{
|
{
|
||||||
"success": True,
|
"success": True,
|
||||||
"current_path": str(path),
|
"current_path": str(path),
|
||||||
"parent_path": str(parent_path) if show_parent else None,
|
"parent_path": parent_path,
|
||||||
"directories": directories,
|
"directories": directories,
|
||||||
"image_files": image_files,
|
"image_files": image_files,
|
||||||
"image_count": len(image_files),
|
"image_count": len(image_files),
|
||||||
@@ -3423,3 +3434,30 @@ class BatchImportHandler:
|
|||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
self._logger.error("Error browsing directory: %s", exc, exc_info=True)
|
self._logger.error("Error browsing directory: %s", exc, exc_info=True)
|
||||||
return web.json_response({"success": False, "error": str(exc)}, status=500)
|
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),
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ export class BatchImportManager {
|
|||||||
this.results = null;
|
this.results = null;
|
||||||
this.isCancelled = false;
|
this.isCancelled = false;
|
||||||
this.isImporting = false;
|
this.isImporting = false;
|
||||||
|
this.currentParentPath = null;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -718,9 +719,10 @@ export class BatchImportManager {
|
|||||||
browser.style.display = isVisible ? 'none' : 'block';
|
browser.style.display = isVisible ? 'none' : 'block';
|
||||||
|
|
||||||
if (!isVisible) {
|
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;
|
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 directoryCount = document.getElementById('batchDirectoryCount');
|
||||||
const imageCount = document.getElementById('batchImageCount');
|
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) {
|
if (currentPathEl) {
|
||||||
currentPathEl.textContent = data.current_path;
|
currentPathEl.textContent = data.current_path;
|
||||||
}
|
}
|
||||||
@@ -811,11 +817,9 @@ export class BatchImportManager {
|
|||||||
`;
|
`;
|
||||||
|
|
||||||
item.addEventListener('click', () => {
|
item.addEventListener('click', () => {
|
||||||
if (isParent) {
|
// The parent entry uses the server-provided parent_path (or the
|
||||||
this.navigateToParentDirectory();
|
// Windows drive-list token) directly — both are plain load targets.
|
||||||
} else {
|
|
||||||
this.loadDirectory(path);
|
this.loadDirectory(path);
|
||||||
}
|
|
||||||
});
|
});
|
||||||
|
|
||||||
return item;
|
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() {
|
navigateToParentDirectory() {
|
||||||
const currentPath = document.getElementById('batchCurrentPath')?.textContent;
|
if (this.currentParentPath) {
|
||||||
if (currentPath) {
|
this.loadDirectory(this.currentParentPath);
|
||||||
// Get parent path using path manipulation
|
|
||||||
const lastSeparator = currentPath.lastIndexOf('/');
|
|
||||||
const parentPath = lastSeparator > 0 ? currentPath.substring(0, lastSeparator) : currentPath;
|
|
||||||
this.loadDirectory(parentPath);
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -858,7 +859,13 @@ export class BatchImportManager {
|
|||||||
const currentPath = document.getElementById('batchCurrentPath')?.textContent;
|
const currentPath = document.getElementById('batchCurrentPath')?.textContent;
|
||||||
const directoryInput = document.getElementById('batchDirectoryInput');
|
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;
|
directoryInput.value = currentPath;
|
||||||
this.toggleDirectoryBrowser(); // Close browser
|
this.toggleDirectoryBrowser(); // Close browser
|
||||||
showToast('toast.recipes.batchImportDirectorySelected', { path: currentPath }, 'success');
|
showToast('toast.recipes.batchImportDirectorySelected', { path: currentPath }, 'success');
|
||||||
|
|||||||
@@ -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('');
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -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
|
||||||
Reference in New Issue
Block a user